Skip to content

[change] Replace third-party JSONField with Django built-in JSONField #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nemesifier merged 1 commit into from
May 12, 2026
Merged

[change] Replace third-party JSONField with Django built-in JSONField #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions openwisp_network_topology/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
from django import forms
from django.contrib import admin, messages
from django.contrib.admin import ModelAdmin
from django.db.models import Q
from django.db.models import Q, TextField
from django.db.models.functions import Cast
from django.template.response import TemplateResponse
from django.urls import re_path, reverse
from django.utils.safestring import mark_safe
Expand Down Expand Up @@ -274,7 +275,7 @@ class NodeAdmin(NodeLinkMixin, BaseAdmin):
form = UserPropertiesForm
change_form_template = "admin/topology/node/change_form.html"
list_display = ["get_name", "organization", "topology", "addresses"]
search_fields = ["addresses", "label", "properties"]
search_fields = ["label", "_addresses_text", "_properties_text"]
list_filter = [
(MultitenantOrgFilter),
(TopologyFilter),
Expand All @@ -292,6 +293,14 @@ class NodeAdmin(NodeLinkMixin, BaseAdmin):
"modified",
]

def get_search_results(self, request, queryset, search_term):
if search_term:
queryset = queryset.annotate(
_addresses_text=Cast("addresses", output_field=TextField()),
_properties_text=Cast("properties", output_field=TextField()),
)
return super().get_search_results(request, queryset, search_term)

def change_view(self, request, object_id, form_url="", extra_context=None):
extra_context = extra_context or {}
link_model = self.model.source_link_set.field.model
Expand All @@ -315,10 +324,24 @@ class LinkAdmin(NodeLinkMixin, BaseAdmin):
search_fields = [
"source__label",
"target__label",
"source__addresses",
"target__addresses",
"properties",
"_source_addresses_text",
"_target_addresses_text",
"_properties_text",
]

def get_search_results(self, request, queryset, search_term):
if search_term:
queryset = queryset.annotate(
_source_addresses_text=Cast(
"source__addresses", output_field=TextField()
),
_target_addresses_text=Cast(
"target__addresses", output_field=TextField()
),
_properties_text=Cast("properties", output_field=TextField()),
)
return super().get_search_results(request, queryset, search_term)

list_display = [
"__str__",
"organization",
Expand Down
29 changes: 18 additions & 11 deletions openwisp_network_topology/base/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

import swapper
from django.core.exceptions import ValidationError
from django.core.serializers.json import DjangoJSONEncoder
from django.db import models
from django.db.models import Q
from django.db.models import JSONField, Q, TextField
from django.db.models.functions import Cast
from django.db.models.signals import post_delete, post_save
from django.dispatch import receiver
from django.utils.timezone import now
from django.utils.translation import gettext_lazy as _
from jsonfield import JSONField
from model_utils import Choices
from model_utils.fields import StatusField
from rest_framework.utils.encoders import JSONEncoder
Expand Down Expand Up @@ -48,16 +49,14 @@ class AbstractLink(ShareableOrgMixin, TimeStampedEditableModel):
properties = JSONField(
default=dict,
blank=True,
load_kwargs={"object_pairs_hook": OrderedDict},
dump_kwargs={"indent": 4, "cls": JSONEncoder},
encoder=DjangoJSONEncoder,
)
user_properties = JSONField(
verbose_name=_("user defined properties"),
help_text=_("If you need to add additional data to this link use this field"),
default=dict,
blank=True,
load_kwargs={"object_pairs_hook": OrderedDict},
dump_kwargs={"indent": 4, "cls": JSONEncoder},
encoder=DjangoJSONEncoder,
)
status_changed = models.DateTimeField(auto_now=True)

Expand Down Expand Up @@ -177,12 +176,20 @@ def get_from_nodes(cls, source, target, topology):
:param topology: Topology instance
:returns: Link object or None
"""
source = '"{}"'.format(source)
target = '"{}"'.format(target)
source_needle = '"{}"'.format(source)
target_needle = '"{}"'.format(target)
qs = cls.objects.annotate(
_source_addresses_text=Cast("source__addresses", output_field=TextField()),
_target_addresses_text=Cast("target__addresses", output_field=TextField()),
)
q = Q(
source__addresses__contains=source, target__addresses__contains=target
) | Q(source__addresses__contains=target, target__addresses__contains=source)
return cls.objects.filter(q).filter(topology=topology).first()
_source_addresses_text__contains=source_needle,
_target_addresses_text__contains=target_needle,
) | Q(
_source_addresses_text__contains=target_needle,
_target_addresses_text__contains=source_needle,
)
return qs.filter(q).filter(topology=topology).first()
Comment thread
coderabbitai[bot] marked this conversation as resolved.

@classmethod
def delete_expired_links(cls):
Expand Down
34 changes: 20 additions & 14 deletions openwisp_network_topology/base/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@

import swapper
from django.core.exceptions import ValidationError
from django.core.serializers.json import DjangoJSONEncoder
from django.db import models
from django.db.models import JSONField, TextField
from django.db.models.functions import Cast
from django.db.models.signals import post_delete, post_save
from django.dispatch import receiver
from django.utils.functional import cached_property
from django.utils.timezone import now
from django.utils.translation import gettext_lazy as _
from jsonfield import JSONField
from rest_framework.utils.encoders import JSONEncoder

from openwisp_users.mixins import ShareableOrgMixin
Expand All @@ -32,20 +34,18 @@ class AbstractNode(ShareableOrgMixin, TimeStampedEditableModel):
)
label = models.CharField(max_length=64, blank=True)
# netjson ID and local_addresses
addresses = JSONField(default=[], blank=True)
addresses = JSONField(default=list, blank=True, encoder=DjangoJSONEncoder)
properties = JSONField(
default=dict,
blank=True,
load_kwargs={"object_pairs_hook": OrderedDict},
dump_kwargs={"indent": 4, "cls": JSONEncoder},
encoder=DjangoJSONEncoder,
)
user_properties = JSONField(
verbose_name=_("user defined properties"),
help_text=_("If you need to add additional data to this node use this field"),
default=dict,
blank=True,
load_kwargs={"object_pairs_hook": OrderedDict},
dump_kwargs={"indent": 4, "cls": JSONEncoder},
encoder=DjangoJSONEncoder,
)

class Meta:
Expand Down Expand Up @@ -138,10 +138,13 @@ def get_from_address(cls, address, topology):
:param topology: Topology instance
:returns: Node object or None
"""
address = '"{}"'.format(address)
return cls.objects.filter(
topology=topology, addresses__contains=address
).first()
needle = '"{}"'.format(address)
return (
cls.objects.filter(topology=topology)
.annotate(_addresses_text=Cast("addresses", output_field=TextField()))
.filter(_addresses_text__contains=needle)
.first()
)
Comment on lines +141 to +147
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


@classmethod
def count_address(cls, address, topology):
Expand All @@ -151,10 +154,13 @@ def count_address(cls, address, topology):
:param topology: Topology instance
:returns: int
"""
address = '"{}"'.format(address)
return cls.objects.filter(
topology=topology, addresses__contains=address
).count()
needle = '"{}"'.format(address)
return (
cls.objects.filter(topology=topology)
.annotate(_addresses_text=Cast("addresses", output_field=TextField()))
.filter(_addresses_text__contains=needle)
.count()
)
Comment on lines +157 to +163
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


@classmethod
def delete_expired_nodes(cls):
Expand Down
22 changes: 11 additions & 11 deletions openwisp_network_topology/integrations/device/tests/test_wifi_mesh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,30 +93,30 @@ def test_simple_mesh(self):
Node.objects.filter(
topology=topology,
organization=org,
properties__contains=(
'{\n "ht": true,\n "vht": null,\n "mfp": false,\n'
' "wmm": true,\n "vendor": "TP-LINK TECHNOLOGIES CO.,LTD."\n}'
),
properties__ht=True,
properties__vht=None,
properties__mfp=False,
properties__wmm=True,
properties__vendor="TP-LINK TECHNOLOGIES CO.,LTD.",
).count(),
3,
)
self.assertEqual(
Link.objects.filter(
topology=topology,
organization=org,
properties__contains='"noise": -94',
)
.filter(properties__contains='"signal": -58')
.filter(properties__contains='"mesh_llid": 19500')
.filter(properties__contains='"mesh_plid": 24500')
.count(),
properties__noise=-94,
properties__signal=-58,
properties__mesh_llid=19500,
properties__mesh_plid=24500,
).count(),
3,
)
self.assertEqual(
Link.objects.filter(
topology=topology,
organization=org,
properties__contains='"mesh_non_peer_ps": "INCONSISTENT: (LISTEN / ACTIVE)"',
properties__mesh_non_peer_ps="INCONSISTENT: (LISTEN / ACTIVE)",
).count(),
1,
)
Expand Down
4 changes: 4 additions & 0 deletions openwisp_network_topology/management/commands/upgrade_from_django_netjsongraph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ def handle(self, *args, **options):
for data in netjsongraph_data:
data["fields"]["organization"] = str(org.pk)
data["model"] = f'{self.app_label}.{data["model"].split(".")[1]}'
for json_field in ("addresses", "properties"):
value = data["fields"].get(json_field)
if isinstance(value, str):
data["fields"][json_field] = json.loads(value)
Comment on lines +82 to +85
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

# Save in anotherfile
with open(f'{options["backup"]}/netjsongraph_loaded.json', "w") as outfile:
json.dump(netjsongraph_data, outfile)
Expand Down
61 changes: 61 additions & 0 deletions openwisp_network_topology/migrations/0017_migrate_to_django_jsonfield.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import django.core.serializers.json
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("topology", "0016_alter_topology_parser"),
]

operations = [
migrations.AlterField(
model_name="link",
name="properties",
field=models.JSONField(
blank=True,
default=dict,
encoder=django.core.serializers.json.DjangoJSONEncoder,
),
),
migrations.AlterField(
model_name="link",
name="user_properties",
field=models.JSONField(
blank=True,
default=dict,
encoder=django.core.serializers.json.DjangoJSONEncoder,
help_text="If you need to add additional data to this link use this field",
verbose_name="user defined properties",
),
),
migrations.AlterField(
model_name="node",
name="addresses",
field=models.JSONField(
blank=True,
default=list,
encoder=django.core.serializers.json.DjangoJSONEncoder,
),
),
migrations.AlterField(
model_name="node",
name="properties",
field=models.JSONField(
blank=True,
default=dict,
encoder=django.core.serializers.json.DjangoJSONEncoder,
),
),
migrations.AlterField(
model_name="node",
name="user_properties",
field=models.JSONField(
blank=True,
default=dict,
encoder=django.core.serializers.json.DjangoJSONEncoder,
help_text="If you need to add additional data to this node use this field",
verbose_name="user defined properties",
),
),
]
Comment thread
coderabbitai[bot] marked this conversation as resolved.
2 changes: 1 addition & 1 deletion openwisp_network_topology/tests/test_link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def test_get_from_nodes(self):
target=node2,
cost=1.0,
cost_text="100mbit/s",
properties='{"pretty": true}',
properties={"pretty": True},
)
link.full_clean()
link.save()
Expand Down
8 changes: 4 additions & 4 deletions openwisp_network_topology/tests/test_topology.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ def test_update_added(self):
t.update()
self.assertEqual(self.node_model.objects.count(), 2)
self.assertEqual(self.link_model.objects.count(), 1)
node1 = self.node_model.objects.get(addresses__contains='"192.168.0.1"')
node2 = self.node_model.objects.get(addresses__contains='"192.168.0.2"')
node1 = self.node_model.get_from_address("192.168.0.1", t)
node2 = self.node_model.get_from_address("192.168.0.2", t)
self.assertEqual(node1.local_addresses, ["10.0.0.1"])
self.assertEqual(node1.properties, {"gateway": True})
link = self.link_model.objects.first()
Expand Down Expand Up @@ -297,8 +297,8 @@ def _test_receive_added(self, expiration_time=0):
t.receive(data)
self.assertEqual(self.node_model.objects.count(), 2)
self.assertEqual(self.link_model.objects.count(), 1)
node1 = self.node_model.objects.get(addresses__contains='"192.168.0.1"')
node2 = self.node_model.objects.get(addresses__contains='"192.168.0.2"')
node1 = self.node_model.get_from_address("192.168.0.1", t)
node2 = self.node_model.get_from_address("192.168.0.2", t)
self.assertEqual(node1.local_addresses, ["10.0.0.1"])
self.assertEqual(node1.properties, {"gateway": True})
link = self.link_model.objects.first()
Expand Down
Loading
Loading