[chores:fix] Preserved text -> jsonb migration for JSONField fields #297#299
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📜 Recent 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). (5)
🧰 Additional context used📓 Path-based instructions (5)**/*.{sh,bash,py,js,ts,tsx,jsx,java,cs,cpp,c,go,rb}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb,sh}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{py,html,txt}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{js,ts,tsx,jsx,py,java,cs}📄 CodeRabbit inference engine (Custom checks)
Files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughThis pull request removes the jsonfield dependency and converts all historical migration fields that used jsonfield.fields.JSONField to django.db.models.TextField across production and test migrations. It also updates migration helper functions to import json and serialize/deserialize node.addresses as JSON strings, and removes jsonfield from requirements.txt. Field attributes like blank and default are preserved where applicable. Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Possibly related issues
Suggested labelsdependencies 🚥 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)
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
added
the
dependencies
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental changes correctly address edge cases identified in the previous review. Summary of Changes Since Previous Review
Files Reviewed in This Incremental Review (5 files)
Reviewed by kimi-k2.5-0127 · 88,723 tokens |
|
coverage: 98.873%. first build — fix-jsonfield-migrations into master |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_network_topology/migrations/0010_properties_json.py`:
- Around line 13-16: Migration 0010 changed node.addresses to a TextField
defaulting to dict which can leave stored values as stringified lists; update
migrate_openvpn_ids_0012 in migration 0012_update_openvpn_netjson_ids.py so it
first detects when node.addresses is a str and deserializes it (e.g., json.loads
or ast.literal_eval) into a Python list before doing node.addresses[0] =
node.label, ensuring you handle non-list/None cases safely and then save the
instance; alternatively, if you prefer, revert 0010 to store JSON-compatible
types, but the minimal fix is to add the deserialization and validation in
migrate_openvpn_ids_0012.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d9d6137-49fc-4294-a587-3c3159223cf3
📒 Files selected for processing (8)
openwisp_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyrequirements.txttests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.py
💤 Files with no reviewable changes (1)
- requirements.txt
📜 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). (13)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{sh,bash,py,js,ts,tsx,jsx,java,cs,cpp,c,go,rb}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb,sh}
📄 CodeRabbit inference engine (Custom checks)
Avoid unnecessary comments or docstrings for code that is already clear
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb}: Code formatting must be compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Variables, functions, classes, and files must have descriptive and consistent names
Errors that cannot be resolved by the user must be logged with error level
Unusual conditions must be logged with warning level
Important background actions must be logged with info level
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests: Ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs}
📄 CodeRabbit inference engine (Custom checks)
Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pyopenwisp_network_topology/migrations/0001_initial.py
Historical migrations were updated to use models.TextField instead of jsonfield.fields.JSONField for fields originally backed by the third-party jsonfield library. The original jsonfield.fields.JSONField inherited from TextField and stored values as PostgreSQL text. Preserving the historical field semantics as TextField (rather than models.JSONField) allows Django to correctly detect the transition from text -> jsonb and generate the required ALTER COLUMN ... TYPE jsonb migration SQL during upgrades. The jsonfield package was also dropped from requirements.txt since no code or migration references it anymore. Closes #297
nemesifier
force-pushed
the
fix-jsonfield-migrations
branch
from
2cf7242 to
e0896c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_network_topology/migrations/__init__.py`:
- Around line 29-38: The migration assumes addresses (node.addresses) decodes to
a non-empty list and does addresses[0] = node.label, which will IndexError on an
empty list; update the logic after json.loads() to ensure addresses is a list
and has at least one element (e.g., if not addresses: append node.label or
insert at index 0) before assigning, then continue to json.dumps into
node.addresses and save; touch the block around json.loads, the addresses[0] =
node.label assignment, and node.addresses = json.dumps(addresses).
In `@openwisp_network_topology/migrations/0007_create_new_address_field.py`:
- Line 16: Change the mutable default list literal to a callable: replace the
TextField default in the migration that defines models.TextField(default=[])
(migration 0007_create_new_address_field) to use default=list instead; this
keeps semantics consistent with other defaults (e.g., default=dict) and is safe
since migration 0008's migrate_addresses populates the field.
In `@openwisp_network_topology/migrations/0010_properties_json.py`:
- Line 21: The TextField in the migration currently uses a mutable default
(default=[]) which can cause shared-state bugs; update the field declaration in
the migration (the line with models.TextField(blank=True, default=[])) to use a
callable default instead (default=list) so each instance gets its own empty list
object while preserving blank=True and existing behavior.
In `@tests/openwisp2/sample_network_topology/migrations/0001_initial.py`:
- Line 239: The migration defines the "addresses" field as
models.TextField(default=[]) which uses a mutable default; change it to use a
callable default by replacing default=[] with default=list on the
models.TextField for the "addresses" field (i.e., update the tuple ("addresses",
models.TextField(default=[])) to use models.TextField(default=list)) so tests
mirror production migrations and avoid shared mutable defaults.
In `@tests/openwisp2/sample_network_topology/migrations/0002_json_properties.py`:
- Line 21: The migration uses a mutable default (models.TextField(blank=True,
default=[])); change the field's default to the callable list by replacing
default=[] with default=list to avoid sharing mutable defaults; update the
models.TextField declaration in the migration (the field definition in
0002_json_properties.py) to use default=list so it matches the production
migration and is safe for Django migrations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fbca7a2-4654-445a-99fa-d5db633c9f15
📒 Files selected for processing (9)
openwisp_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pyrequirements.txttests/openwisp2/sample_network_topology/migrations/0001_initial.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.py
💤 Files with no reviewable changes (1)
- requirements.txt
📜 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.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.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~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{sh,bash,py,js,ts,tsx,jsx,java,cs,cpp,c,go,rb}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb,sh}
📄 CodeRabbit inference engine (Custom checks)
Avoid unnecessary comments or docstrings for code that is already clear
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,cs,cpp,c,go,rb}: Code formatting must be compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Variables, functions, classes, and files must have descriptive and consistent names
Errors that cannot be resolved by the user must be logged with error level
Unusual conditions must be logged with warning level
Important background actions must be logged with info level
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests: Ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0001_initial.py
**/*.{js,ts,tsx,jsx,py,java,cs}
📄 CodeRabbit inference engine (Custom checks)
Provide user-facing messages for errors that the user can solve autonomously (for example, validation errors)
Files:
openwisp_network_topology/migrations/0007_create_new_address_field.pyopenwisp_network_topology/migrations/0010_properties_json.pytests/openwisp2/sample_network_topology/migrations/0002_json_properties.pytests/openwisp2/sample_network_topology/migrations/0001_initial.pyopenwisp_network_topology/migrations/0013_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/__init__.pytests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.pyopenwisp_network_topology/migrations/0001_initial.py
🔇 Additional comments (7)
openwisp_network_topology/migrations/0001_initial.py (1)
67-71: LGTM!Also applies to: 115-119
openwisp_network_topology/migrations/0010_properties_json.py (1)
13-16: LGTM!Also applies to: 26-29
openwisp_network_topology/migrations/0013_add_user_defined_properties_field.py (1)
13-18: LGTM!Also applies to: 23-28
openwisp_network_topology/migrations/__init__.py (1)
2-3: LGTM!Also applies to: 21-22
tests/openwisp2/sample_network_topology/migrations/0001_initial.py (1)
242-246: LGTM!Also applies to: 310-314
tests/openwisp2/sample_network_topology/migrations/0002_json_properties.py (1)
13-16: LGTM!Also applies to: 26-29
tests/openwisp2/sample_network_topology/migrations/0003_add_user_defined_properties_field.py (1)
13-18: LGTM!Also applies to: 23-28
- Replaced default=[] with default=list in migrations for consistency with default=dict already used in the same files and to avoid the mutable-literal-as-default anti-pattern. - Guarded migrate_openvpn_ids_0012 against an empty addresses list so it falls back to [node.label] instead of raising IndexError. Related to #299
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@nemesifier does the change to models.JSONField handle this?
The second problem is more subtle and it only shows up on PostgreSQL, which is what most production deployments use. In openwisp_network_topology/admin.py the NodeAdmin declares search_fields = ["addresses", "label", "properties"], and the device integration in openwisp_network_topology/integrations/device/overrides.py adds source__addresses and target__addresses to a similar list. With the old third party JSONField these columns were stored as plain text, so the icontains lookup that the admin search uses under the hood translated to a normal ILIKE and worked fine. With the built in JSONField the column type becomes jsonb on PostgreSQL, and PostgreSQL does not define an ILIKE operator for jsonb, so as soon as someone uses the admin search box on the Node list view they will get an error along the lines of operator does not exist: jsonb ~~* unknown. Our test suite runs on SpatiaLite where the column is still text under the hood, so the regression will not be caught by CI. We need to either drop the JSON columns from the search fields or cast them to text in the search. The same review should be done on the parallel controller and radius PRs.
Quoted from #259 (review)
I double checked on a prod instance and the |
3 participants
Historical migrations were updated to use models.TextField instead of jsonfield.fields.JSONField for fields originally backed by the third-party jsonfield library.
The original jsonfield.fields.JSONField inherited from TextField and stored values as PostgreSQL text. Preserving the historical field semantics as TextField (rather than models.JSONField) allows Django to correctly detect the transition from text -> jsonb and generate the required ALTER COLUMN ... TYPE jsonb migration SQL during upgrades.
The jsonfield package was also dropped from requirements.txt since no code or migration references it anymore.
Closes #297