[change:radius] Replace third-party JSONField with Django built-in JSONField#619
[change:radius] Replace third-party JSONField with Django built-in JSONField#619Eeshu-Yadav wants to merge 2 commits intoopenwisp:masterfrom
Conversation
a9ad394 to
998b688
Compare
a0d7e52 to
21b4925
Compare
665a8b5 to
fa44638
Compare
|
@nemesifier kindly review |
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @Eeshu-Yadav, see my comments below.
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py
Outdated
Show resolved
Hide resolved
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py
Outdated
Show resolved
Hide resolved
openwisp_radius/migrations/0043_alter_organizationradiussettings_sms_meta_data_and_more.py
Outdated
Show resolved
Hide resolved
fa44638 to
a763c2b
Compare
a763c2b to
ba82cd9
Compare
|
@nemesifier kindly review |
ba82cd9 to
1c67f5d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces third-party jsonfield.JSONField with Django's built-in models.JSONField across models, tests, and migrations; adds DjangoJSONEncoder to JSONField declarations; removes jsonfield from setup.py. Updates imports in openwisp_radius/base/models.py and changes AbstractRadiusBatch.prefix_add to assign Python objects (not JSON strings) into user_credentials. Adds schema migrations to alter JSONField metadata and a data migration that iterates RadiusBatch records, converts double-encoded JSON strings in user_credentials into proper JSON objects, and saves the normalized values. 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 | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
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.
There was a problem hiding this comment.
Ok but this is not the right way of doing this, we should:
try/except and if the exception is reached: call log.exception() and print an error message with the exception message, eg:
print("Encountered error while processing {batch}: {e}")
1c67f5d to
3f0f101
Compare
|
@nemesifier kindly review |
3f0f101 to
88f4e34
Compare
| 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.
Ok but this is not the right way of doing this, we should:
try/except and if the exception is reached: call log.exception() and print an error message with the exception message, eg:
print("Encountered error while processing {batch}: {e}")
88f4e34 to
5621afc
Compare
7e73208 to
930abf0
Compare
…#600 Replaced jsonfield package's JSONField with Django's built-in JSONField across all models to use the modern, maintained Django implementation. 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 and migration 0044 to convert existing double-encoded JSON data in user_credentials field with proper error handling. Used .iterator() in data migration for memory efficiency and removed jsonfield dependency from setup.py. Closes openwisp#600
930abf0 to
cdd7769
Compare
11359eb to
b07ac97
Compare
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: