[change:radius] Replace third-party JSONField with Django built-in JSONField #619
Conversation
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
2 times, most recently
from
a9ad394 to
998b688
Compare
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
2 times, most recently
from
a0d7e52 to
21b4925
Compare
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
4 times, most recently
from
665a8b5 to
fa44638
Compare
|
@nemesifier kindly review |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @Eeshu-Yadav, see my comments below.
| name="user_credentials", | ||
| field=models.JSONField(blank=True, null=True, verbose_name="PDF"), | ||
| ), | ||
| migrations.RunPython( |
There was a problem hiding this comment.
Data migrations need to be executed in a dedicated migration file, please refer to https://stackoverflow.com/a/66799446 for more information.
| batch.user_credentials = json.loads(batch.user_credentials) | ||
| batch.save(update_fields=["user_credentials"]) | ||
| except (json.JSONDecodeError, TypeError): | ||
| # If it's already decoded or invalid JSON, skip it |
There was a problem hiding this comment.
It shouldn't fail. If it fails we must let it fail loudly, unless there can be failure which would be common and in that case we should catch the exception and resolve this automatically.
Why do you think it would fail here?
| to proper JSON objects for Django's built-in JSONField. | ||
| """ | ||
| RadiusBatch = apps.get_model("openwisp_radius", "RadiusBatch") | ||
| for batch in RadiusBatch.objects.exclude(user_credentials__isnull=True): |
There was a problem hiding this comment.
| for batch in RadiusBatch.objects.exclude(user_credentials__isnull=True): | |
| for batch in RadiusBatch.objects.exclude(user_credentials__isnull=True).iterator(): |
Not using iterator() may crash instances with large amount of rows and must be avoided.
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
from
fa44638 to
a763c2b
Compare
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
from
a763c2b to
ba82cd9
Compare
|
@nemesifier kindly review |
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
from
ba82cd9 to
1c67f5d
Compare
📝 WalkthroughWalkthroughThis PR replaces a third-party jsonfield.JSONField with Django's built-in models.JSONField across models and migrations, removes the jsonfield dependency from setup.py, updates imports in models, changes AbstractRadiusBatch.prefix_add to store Python objects (not JSON strings) in user_credentials, adds a schema migration adjusting JSONField options, and adds a data migration that converts existing double-encoded JSON strings in radiusbatch.user_credentials into proper JSON objects. Sequence Diagram(s)sequenceDiagram
participant Migration as Migration (RunPython)
participant ORM as ORM (apps.get_model)
participant DB as Database
participant JSON as json.loads
Migration->>ORM: get RadiusBatch model
Migration->>DB: query RadiusBatch objects (exclude null user_credentials)
DB-->>Migration: iterator of RadiusBatch rows
loop per RadiusBatch
Migration->>Migration: read instance.user_credentials
alt value is string
Migration->>JSON: json.loads(string)
JSON-->>Migration: python_obj
Migration->>DB: save instance with user_credentials=python_obj (update_fields)
DB-->>Migration: acknowledge save
else value already object
Migration-->>Migration: skip
end
end
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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)openwisp_radius/migrations/0044_convert_user_credentials_data.py (2)
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py (2)
🪛 Ruff (0.14.14)openwisp_radius/migrations/0044_convert_user_credentials_data.py[warning] 27-32: Mutable class attributes should be annotated with (RUF012) [warning] 34-38: Mutable class attributes should be annotated with (RUF012) openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py[warning] 8-10: Mutable class attributes should be annotated with (RUF012) [warning] 12-31: 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 (5)
✏️ 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 |
![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
🤖 Fix all issues with AI agents
In `@openwisp_radius/migrations/0044_convert_user_credentials_data.py`:
- Around line 8-17: The migration's DB operations must be routed to the
migration database: obtain db_alias = schema_editor.connection.alias inside
convert_user_credentials_data and use it when querying and saving RadiusBatch
(e.g., use RadiusBatch.objects.using(db_alias).exclude(...).iterator() and call
batch.save(update_fields=["user_credentials"], using=db_alias)) so reads/writes
go to the correct database during migrations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_radius/base/models.pyopenwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.pyopenwisp_radius/migrations/0044_convert_user_credentials_data.pysetup.pytests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py
💤 Files with no reviewable changes (1)
- setup.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py (1)
openwisp_radius/migrations/0044_convert_user_credentials_data.py (1)
Migration(20-33)
🪛 Ruff (0.14.14)
openwisp_radius/migrations/0044_convert_user_credentials_data.py
[warning] 8-8: Unused function argument: schema_editor
(ARG001)
[warning] 22-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 29-33: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-31: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: Python==3.12 | django~=5.2.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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🔇 Additional comments (4)
openwisp_radius/base/models.py (2)
20-20: Import adjustment looks fine.Line 20 change is consistent with the rest of the file.
1017-1023: prefix_add update looks consistent.Line 1022 assignment aligns with the surrounding logic in
prefix_add.openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py (1)
13-30: Migration update looks good.The field alterations are straightforward and consistent.
tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py (1)
544-553: Sample migration updates look good.These updates are consistent with the rest of the migration.
Also applies to: 696-698
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openwisp_radius/migrations/0044_convert_user_credentials_data.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request migrates from the unmaintained third-party jsonfield library to Django's built-in JSONField. The third-party library hasn't been updated in 5 years, while Django's JSONField is actively maintained and natively supported across all databases used by the project.
Changes:
- Replaced third-party JSONField with Django's built-in
models.JSONFieldin model definitions - Removed
jsonfield~=3.1.0dependency from setup.py - Added migrations to handle the field type transition and data conversion
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Removed the deprecated jsonfield dependency |
| openwisp_radius/base/models.py | Updated imports to use Django's JSONField and removed manual json.dumps() call that was causing double-encoding |
| openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py | AlterField migration to change field types from third-party JSONField to Django's JSONField |
| openwisp_radius/migrations/0044_convert_user_credentials_data.py | Data migration to convert double-encoded JSON strings to proper Python objects |
| tests/openwisp2/sample_radius/migrations/0002_initial_openwisp_app.py | Updated test migrations to use Django's JSONField |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RadiusBatch = apps.get_model("openwisp_radius", "RadiusBatch") | ||
| for batch in RadiusBatch.objects.exclude(user_credentials__isnull=True).iterator(): | ||
| if isinstance(batch.user_credentials, str): | ||
| batch.user_credentials = json.loads(batch.user_credentials) |
There was a problem hiding this comment.
The migration should add error handling for edge cases where user_credentials might be an empty string or invalid JSON. Currently, if user_credentials is an empty string, json.loads will raise a JSONDecodeError and fail the migration. Consider adding a try-except block to handle this gracefully, or add an additional check to skip empty strings before attempting to parse.
| batch.user_credentials = json.loads(batch.user_credentials) | |
| # Skip empty or whitespace-only strings which would fail json.loads | |
| if not batch.user_credentials.strip(): | |
| continue | |
| # Safely attempt to decode JSON; skip records with invalid JSON | |
| try: | |
| decoded_credentials = json.loads(batch.user_credentials) | |
| except json.JSONDecodeError: | |
| continue | |
| batch.user_credentials = decoded_credentials |
There was a problem hiding this comment.
@nemesifier previously requested that this migration "fail loudly" on invalid data rather than silently skip errors, to ensure data integrity issues are caught and investigated rather than hidden. If empty strings or invalid JSON turn out to be common during testing, we can address them explicitly with proper handling.
- Replaced third-party jsonfield.JSONField with Django's built-in models.JSONField in all model definitions - Updated RadiusBatch.prefix_add method to directly assign user_credentials list instead of using json.dumps() - Added migration 0043 to replace JSONField in model definitions - Added migration 0044 to convert existing double-encoded JSON data in user_credentials field (data migration in separate file) - Used .iterator() in data migration for memory efficiency - Removed jsonfield dependency from setup.py - Updated test migrations to use Django's JSONField - Removed unused json import Closes openwisp#600
Eeshu-Yadav
force-pushed
the
issues/600-replace-jsonfield-clean
branch
from
1c67f5d to
3f0f101
Compare
|
@nemesifier kindly review |
3 participants
Checklist
Reference to Existing Issue
Closes #600.
Description of Changes
This PR replaces the third-party
jsonfieldlibrary with Django's built-in JSONField. The third-party JSONField library hasn't received updates in 5 years, while Django's built-in JSONField is now mature and actively maintained.Changes Made:
from jsonfield import JSONFieldwithfrom django.db.models import JSONFieldin modelsRadiusBatch.user_credentialsandOrganizationRadiusSettings.sms_meta_datafields"jsonfield~=3.1.0"dependency from setup.py0041_replace_jsonfield_with_django_builtin.pyto handle field type transitionTesting: