[change] Replace third-party JSONField with Django built-in JSONField#259
Conversation
nemesifier
changed the title
|
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 (9)
📝 WalkthroughWalkthroughThis pull request migrates the OpenWISP Network Topology application from the unmaintained third-party Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Black Formatting ErrorsHello @Eeshu-Yadav and @nemesifier, The CI failed because of formatting issues detected by Black. openwisp-qa-format |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks for working on this. I went through the diff carefully and I think there are a couple of things that need to be addressed before we can merge, otherwise we risk breaking installations.
The first problem is that the jsonfield package has been removed from requirements.txt, but several of the existing migration files still import it at the top of the file. For example 0001_initial.py, 0007_create_new_address_field.py, 0010_properties_json.py and 0013_add_user_defined_properties_field.py all start with import jsonfield.fields, and the same is true for the sample test app in tests/openwisp2/sample_network_topology/migrations/. When Django runs migrate or makemigrations on a fresh environment it imports every migration file in the graph, so if the package is no longer installed those imports will fail with a ModuleNotFoundError and the whole install will be unusable. The way we handled the same change in openwisp controller was to keep jsonfield listed in requirements.txt precisely so that the old migrations keep loading. We can drop it for real later, once the historical migrations get squashed. For now please put it back into requirements.txt.
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.
A couple of smaller things worth noting while we are at it. When the migration runs on an existing PostgreSQL database it will perform an ALTER COLUMN ... TYPE jsonb USING column::jsonb on the affected tables, which is fine as long as every existing row holds valid JSON. The old library always serialized via json.dumps so in theory we should be safe, but it would be reassuring to test this against a real production snapshot before release, and to mention the column type change in CHANGES.rst so that operators are aware. Also, by removing dump_kwargs={\"indent\": 4, \"cls\": JSONEncoder} we are implicitly switching the JSON encoder from the one provided by Django REST Framework to DjangoJSONEncoder. The DRF encoder handles a few additional types like lazy translation strings, hyperlinks and ReturnDict. It is unlikely that any of these ever ends up inside properties or user_properties, but it is worth keeping in mind in case something subtle breaks.
On the positive side, the rest of the change looks clean. The switch from default=[] to default=list on node.addresses is the right fix, the new migration files declare the correct dependencies on the previous migrations, the OrderedDict and JSONEncoder imports that remain in base/link.py and base/node.py are still used elsewhere in those modules so they are correctly kept, and no data migration is needed because the JSON payloads themselves do not change.
So in short, please restore jsonfield in requirements.txt, decide what to do about the JSON fields appearing in admin search, and add a note in the changelog about the PostgreSQL column type change. Once these are addressed I think we are good to go.
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
Eeshu-Yadav
force-pushed
the
issues/253-replace-thirdparty-jsonfield
branch
from
49c6428 to
a04d204
Compare
Code Review SummaryStatus: Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Explanation: The incremental diff shows that all changes have been reverted:
The PR no longer achieves its stated goal of "Replace third-party JSONField with Django built-in JSONField" (Issue #253). The current state is identical to the original codebase before the PR. Files ReviewedThe incremental diff shows all 8 files have been reverted to their original state:
Reviewed by kimi-k2.5 · 129,183 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/base/link.py`:
- Around line 179-192: The code builds source_needle and target_needle as
'"{}"'.format(...) and casts JSON address fields to text via Cast into
_source_addresses_text/_target_addresses_text to perform PostgreSQL jsonb
element matching in the Link lookup; add a concise inline comment above the
source_needle/target_needle and annotate/Cast block explaining that the
quote-wrapping ensures matching a JSON array element (e.g. "addr") when the
jsonb is cast to text and that the subsequent Q queries check both source→target
and target→source directions for topology-scoped links (referencing
source_needle, target_needle, Cast(..., output_field=TextField()),
_source_addresses_text, _target_addresses_text, and the final
qs.filter(...).first()).
In `@openwisp_network_topology/base/node.py`:
- Around line 157-163: Add a brief comment above the needle =
'"{}"'.format(address) line in the class method that counts addresses (the
method using
cls.objects.filter(...).annotate(...).filter(_addresses_text__contains=needle).count())
explaining that addresses is stored as a JSON/text field and the code wraps the
address in double quotes to match the exact JSON string value (preventing
partial substring matches, e.g. "1.2.3.4" matching "11.2.3.44"), and reference
that this mirrors the behavior in get_from_address so maintainers understand the
pattern and rationale.
- Around line 141-147: Explain the quote-wrapping intent and potential
false-positive risk by adding a concise comment above the needle =
'"{}"'.format(address) line: state that addresses are stored as JSON strings and
we wrap the address in double-quotes to match JSON array elements, and note that
the substring filter may match prefixes (e.g., "192.168.0.1" vs "192.168.0.10").
Then add an exact-match safeguard after the DB query (in the same classmethod
using cls.objects.filter(...).first()) — e.g., load/parse the returned node's
addresses JSON and verify the address is present, or use a stricter matching
approach — so the method (referencing needle, Cast("addresses",
output_field=TextField()), and the cls.objects...filter(...).first() call) only
returns a node if the address is an exact element.
In
`@openwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.py`:
- Around line 82-85: Wrap the json.loads call in a try/except that catches
json.JSONDecodeError (and ValueError for older Python if needed) around the
block that handles json_field in the loop; on exception, log a warning including
identifying info from the record (e.g., data["pk"] and json_field) and leave the
field in a safe state (e.g., set data["fields"][json_field] = None or keep the
original string) so a single malformed record doesn't abort the entire upgrade;
update the loop handling the variables json_field, value, and data["fields"]
accordingly and ensure the logger/imports used for logging are available.
In `@openwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.py`:
- Around line 1-61: The migration file 0017_migrate_to_django_jsonfield.py
updates Link/Node JSON fields via migrations.AlterField (class Migration,
operations targeting model_name "link" fields properties and user_properties and
model_name "node" fields addresses, properties, user_properties); ensure you run
these migrations against real test databases (PostgreSQL and SQLite) before
deployment to verify existing JSON data is preserved and behavior matches the
test app's 0005_migrate_to_django_jsonfield.py; if any incompatibilities are
found, adjust the AlterField defaults/encoders or add a data migration to
transform stored values, and document a rollback plan (how to restore
schema/data and revert migration) so you can revert
0017_migrate_to_django_jsonfield.py safely in production.
🪄 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: 174abcf0-893d-496c-ac89-7d8aee06b836
📒 Files selected for processing (8)
openwisp_network_topology/admin.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/tests/test_topology.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.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). (13)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.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.11 | django~=5.2.0
- GitHub Check: Python==3.11 | 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.12 | django~=4.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/admin.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/tests/test_topology.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.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/admin.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/tests/test_topology.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.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/admin.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/tests/test_topology.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.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/admin.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/tests/test_topology.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.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/admin.pyopenwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.pyopenwisp_network_topology/tests/test_topology.pyopenwisp_network_topology/tests/test_link.pyopenwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.pytests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.pyopenwisp_network_topology/base/link.pyopenwisp_network_topology/base/node.py
🔇 Additional comments (7)
openwisp_network_topology/tests/test_link.py (1)
121-121: LGTM!openwisp_network_topology/tests/test_topology.py (1)
158-159: LGTM!Also applies to: 300-301
openwisp_network_topology/base/node.py (1)
37-49: LGTM!openwisp_network_topology/admin.py (2)
278-278: LGTM!Also applies to: 296-302
327-330: LGTM!Also applies to: 332-343
openwisp_network_topology/base/link.py (1)
52-52: LGTM!Also applies to: 59-59
tests/openwisp2/sample_network_topology/migrations/0005_migrate_to_django_jsonfield.py (1)
1-61: LGTM!
| needle = '"{}"'.format(address) | ||
| return ( | ||
| cls.objects.filter(topology=topology) | ||
| .annotate(_addresses_text=Cast("addresses", output_field=TextField())) | ||
| .filter(_addresses_text__contains=needle) | ||
| .first() | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add comment explaining the quote-wrapping pattern and consider exact-match validation.
The needle = '"{}"'.format(address) pattern wraps the address in double quotes to match JSON array syntax, but this is not immediately obvious and could lead to false positives (e.g., searching for "192.168.0.1" would also match "192.168.0.10" in the serialized JSON).
📝 Proposed improvements
`@classmethod`
def get_from_address(cls, address, topology):
"""
Find node from one of its addresses and its topology.
:param address: string
:param topology: Topology instance
:returns: Node object or None
"""
+ # Wrap address in quotes to match JSON array element syntax: ["192.168.0.1"]
+ # Cast to text for substring search (required for PostgreSQL jsonb compatibility)
needle = '"{}"'.format(address)
return (
cls.objects.filter(topology=topology)
.annotate(_addresses_text=Cast("addresses", output_field=TextField()))
.filter(_addresses_text__contains=needle)
.first()
)Additionally, consider if substring matching could cause false positives. For example, searching for "192.168.0.1" would match both ["192.168.0.1"] and ["192.168.0.10"]. If this is a concern, you may want to add validation that the returned node actually contains the exact address.
As per coding guidelines, cryptic or non-obvious code (complex patterns or hard-to-read code) must include a concise comment explaining why it is needed.
🤖 Prompt for 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.
In `@openwisp_network_topology/base/node.py` around lines 141 - 147, Explain the
quote-wrapping intent and potential false-positive risk by adding a concise
comment above the needle = '"{}"'.format(address) line: state that addresses are
stored as JSON strings and we wrap the address in double-quotes to match JSON
array elements, and note that the substring filter may match prefixes (e.g.,
"192.168.0.1" vs "192.168.0.10"). Then add an exact-match safeguard after the DB
query (in the same classmethod using cls.objects.filter(...).first()) — e.g.,
load/parse the returned node's addresses JSON and verify the address is present,
or use a stricter matching approach — so the method (referencing needle,
Cast("addresses", output_field=TextField()), and the
cls.objects...filter(...).first() call) only returns a node if the address is an
exact element.
| needle = '"{}"'.format(address) | ||
| return ( | ||
| cls.objects.filter(topology=topology) | ||
| .annotate(_addresses_text=Cast("addresses", output_field=TextField())) | ||
| .filter(_addresses_text__contains=needle) | ||
| .count() | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add comment explaining the quote-wrapping pattern.
Similar to get_from_address, the quote-wrapping pattern here needs explanation for maintainability.
📝 Proposed improvement
`@classmethod`
def count_address(cls, address, topology):
"""
Count nodes with the specified address and topology.
:param address: string
:param topology: Topology instance
:returns: int
"""
+ # Wrap address in quotes to match JSON array element syntax: ["192.168.0.1"]
+ # Cast to text for substring search (required for PostgreSQL jsonb compatibility)
needle = '"{}"'.format(address)
return (
cls.objects.filter(topology=topology)
.annotate(_addresses_text=Cast("addresses", output_field=TextField()))
.filter(_addresses_text__contains=needle)
.count()
)As per coding guidelines, cryptic or non-obvious code (complex patterns or hard-to-read code) must include a concise comment explaining why it is needed.
🤖 Prompt for 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.
In `@openwisp_network_topology/base/node.py` around lines 157 - 163, Add a brief
comment above the needle = '"{}"'.format(address) line in the class method that
counts addresses (the method using
cls.objects.filter(...).annotate(...).filter(_addresses_text__contains=needle).count())
explaining that addresses is stored as a JSON/text field and the code wraps the
address in double quotes to match the exact JSON string value (preventing
partial substring matches, e.g. "1.2.3.4" matching "11.2.3.44"), and reference
that this mirrors the behavior in get_from_address so maintainers understand the
pattern and rationale.
| for json_field in ("addresses", "properties"): | ||
| value = data["fields"].get(json_field) | ||
| if isinstance(value, str): | ||
| data["fields"][json_field] = json.loads(value) |
There was a problem hiding this comment.
Add error handling for JSON parsing.
The json.loads() call can raise JSONDecodeError if the legacy data contains malformed JSON strings. This could cause the entire upgrade to fail for one bad record.
🛡️ Proposed fix to add error handling
for json_field in ("addresses", "properties"):
value = data["fields"].get(json_field)
if isinstance(value, str):
- data["fields"][json_field] = json.loads(value)
+ try:
+ data["fields"][json_field] = json.loads(value)
+ except json.JSONDecodeError:
+ self.stdout.write(
+ self.style.WARNING(
+ f"Failed to parse {json_field} for {data['model']} "
+ f"(pk: {data['pk']}), using empty default"
+ )
+ )
+ data["fields"][json_field] = [] if json_field == "addresses" else {}🤖 Prompt for 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.
In
`@openwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.py`
around lines 82 - 85, Wrap the json.loads call in a try/except that catches
json.JSONDecodeError (and ValueError for older Python if needed) around the
block that handles json_field in the loop; on exception, log a warning including
identifying info from the record (e.g., data["pk"] and json_field) and leave the
field in a safe state (e.g., set data["fields"][json_field] = None or keep the
original string) so a single malformed record doesn't abort the entire upgrade;
update the loop handling the variables json_field, value, and data["fields"]
accordingly and ensure the logger/imports used for logging are available.
Test Failures in Wifi Mesh IntegrationHello @Eeshu-Yadav, The CI failed due to a To resolve this, you should ensure that the test database configuration uses a backend that supports the The CI also reported several slow tests:
While these are not blocking failures, it would be beneficial to investigate and optimize these tests to improve CI performance. |
…penwisp#253 jsonfield is kept in requirements.txt because historical migration files still import it; it can be dropped once those migrations are squashed. JSON columns in admin search_fields are cast to text in get_search_results to avoid the unsupported ILIKE operator on PostgreSQL jsonb. The same cast is applied to the __contains lookups in Node.get_from_address, Node.count_address and Link.get_from_nodes so they keep working on SQLite/SpatiaLite. The upgrade_from_django_netjsongraph command JSON-decodes the legacy addresses/properties string fields before loading them, so they are stored as proper JSON values by Django's JSONField. Fixes openwisp#253
Eeshu-Yadav
force-pushed
the
issues/253-replace-thirdparty-jsonfield
branch
from
a04d204 to
a10a27a
Compare
|
coverage: 98.872% (-0.1%) from 98.987% — Eeshu-Yadav:issues/253-replace-thirdparty-jsonfield into openwisp:master |
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
Checklist
Reference to Existing Issue
Closes #253.
Description of Changes
Models Updated:
openwisp_network_topology/base/node.py: Updated JSONField usage inaddresses,properties, anduser_propertiesfieldsopenwisp_network_topology/base/link.py: Updated JSONField usage inpropertiesanduser_propertiesfields