[bug] Fixed bug related to addresses formatting #58

django_netjsongraph/base/link.py

@@ -97,8 +97,8 @@ def get_from_nodes(cls, source, target, topology):
         :param topology: Topology instance
         :returns: Link object or None
         """
-        source = '{0};'.format(source)
-        target = '{0};'.format(target)
+        source = ';{0};'.format(source)
+        target = ';{0};'.format(target)
         q = (Q(source__addresses__contains=source,
                target__addresses__contains=target) |
              Q(source__addresses__contains=target,

django_netjsongraph/base/node.py

@@ -40,15 +40,15 @@ def save(self, *args, **kwargs):
 
     def _format_addresses(self):
         """
-        Ensure address format is correct:
-            addr1; addr2; addr3;
+        Ensure address format is correct: ";addr1;addr2;addr3;"
         """
         self.addresses = self.addresses.replace(',', ';')\
                                        .replace(' ', '')\
-                                       .replace(';', '; ')
-        if not self.addresses.endswith('; '):
-            self.addresses += '; '
-        self.addresses = self.addresses[0:-1]
+                                       .replace(';', ';')
+        if not self.addresses.startswith(';'):
+            self.addresses = ';' + self.addresses
+        if not self.addresses.endswith(';'):
+            self.addresses += ';'
 
     def truncate_addresses(self):
         """
@@ -59,13 +59,16 @@ def truncate_addresses(self):
             return
         addresses = self.address_list
         # +1 stands for the character added in self._format_address()
-        while len('; '.join(addresses))+1 > max_length:
+        while len(';'.join(addresses)) + 2 > max_length:
             addresses.pop()
-        self.addresses = '; '.join(addresses)
+        self.addresses = ';'.join(addresses)
 
     @cached_property
     def address_list(self):
-        return self.addresses.replace(' ', '')[0:-1].split(';')
+        addresses = self.addresses.replace(' ', '')
+        if addresses.startswith(';'):
+            addresses = addresses[1:]
+        return addresses[0:-1].split(';')
 
     @property
     def netjson_id(self):
@@ -106,7 +109,7 @@ def get_from_address(cls, address, topology):
         :param topology: Topology instance
         :returns: Node object or None
         """
-        address = '{0};'.format(address)
+        address = ';{0};'.format(address)
         return cls.objects.filter(topology=topology,
                                   addresses__contains=address).first()
 
@@ -118,6 +121,6 @@ def count_address(cls, address, topology):
         :param topology: Topology instance
         :returns: int
         """
-        address = '{0};'.format(address)
+        address = ';{0};'.format(address)
         return cls.objects.filter(topology=topology,
                                   addresses__contains=address).count()

django_netjsongraph/migrations/0006_reformat_addresses.py

@@ -0,0 +1,30 @@
+from __future__ import unicode_literals
+
+from django.db import migrations
+from django_netjsongraph.models import Node
+
+
+def reformat_address_forward(apps, schema_editor):
+    for node in Node.objects.all():
+        if not node.addresses.startswith(';'):
+            node.addresses = ';{0}'.format(node.addresses)
+            node.save()
+
+
+def reformat_address_backward(apps, schema_editor):
+    fake_node_model = apps.get_model('django_netjsongraph', 'Node')
+    for node in fake_node_model.objects.all():
+        if node.addresses.startswith(';'):
+            node.addresses = node.addresses[1:]
+            node.save()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('django_netjsongraph', '0005_snapshot'),
+    ]
+
+    operations = [
+        migrations.RunPython(reformat_address_forward,
+                             reformat_address_backward),
+    ]

django_netjsongraph/tests/base/test_node.py

@@ -21,7 +21,7 @@ def test_node_address_list_single(self):
                            properties=None)
         n.full_clean()
         n.save()
-        self.assertEqual(n.addresses, '192.168.0.1;')
+        self.assertEqual(n.addresses, ';192.168.0.1;')
         self.assertEqual(n.address_list, ['192.168.0.1'])
 
     def test_node_address_list_semicolon(self):
@@ -30,7 +30,7 @@ def test_node_address_list_semicolon(self):
                            addresses='192.168.0.1;')
         n.full_clean()
         n.save()
-        self.assertEqual(n.addresses, '192.168.0.1;')
+        self.assertEqual(n.addresses, ';192.168.0.1;')
         self.assertEqual(n.address_list, ['192.168.0.1'])
 
     def test_node_address_list_multiple(self):
@@ -39,8 +39,10 @@ def test_node_address_list_multiple(self):
                            addresses='192.168.0.1; 10.0.0.1,10.0.0.2;10.0.0.3')
         n.full_clean()
         n.save()
-        self.assertEqual(n.addresses, '192.168.0.1; 10.0.0.1; '
-                                      '10.0.0.2; 10.0.0.3;')
+        # repeat _format_addresses() to ensure idempotency
+        n._format_addresses()
+        self.assertEqual(n.addresses, ';192.168.0.1;10.0.0.1;'
+                                      '10.0.0.2;10.0.0.3;')
         self.assertEqual(n.address_list, ['192.168.0.1',
                                           '10.0.0.1',
                                           '10.0.0.2',
@@ -63,7 +65,7 @@ def test_node_name(self):
     def test_json(self):
         t = self.topology_model.objects.first()
         n = t._create_node(label='test node',
-                           addresses='192.168.0.1;10.0.0.1;',
+                           addresses=';192.168.0.1;10.0.0.1;',
                            properties='{"gateway": true}')
         self.assertEqual(dict(n.json(dict=True)), {
             'id': '192.168.0.1',
@@ -80,13 +82,24 @@ def test_json(self):
     def test_get_from_address(self):
         t = self.topology_model.objects.first()
         n = t._create_node(addresses='192.168.0.1,10.0.0.1')
+        n.full_clean()
         n.save()
         self.assertIsInstance(self.node_model.get_from_address('192.168.0.1', t), self.node_model)
         self.assertIsInstance(self.node_model.get_from_address('10.0.0.1', t), self.node_model)
         self.assertIsNone(self.node_model.get_from_address('wrong', t))
 
     def test_count_address(self):
         t = self.topology_model.objects.first()
-        t._create_node(addresses='192.168.0.1,10.0.0.1')
         self.assertEqual(self.node_model.count_address('192.168.0.1', t), 1)
         self.assertEqual(self.node_model.count_address('0.0.0.0', t), 0)
+
+    def test_count_address_issue_58(self):
+        t = self.topology_model.objects.first()
+        n1 = t._create_node(addresses='Benz_Kalloni')
+        n1.full_clean()
+        n1.save()
+        n2 = t._create_node(addresses='Kalloni')
+        n2.full_clean()
+        n2.save()
+        self.assertEqual(self.node_model.count_address('Benz_Kalloni', t), 1)
+        self.assertEqual(self.node_model.count_address('Kalloni', t), 1)

django_netjsongraph/tests/base/test_topology.py

@@ -378,7 +378,7 @@ def test_very_long_addresses(self):
         data = self._load('static/very-long-addresses.json')
         t.receive(data)
         n = self.node_model.get_from_address('2001:4e12:452a:1:172::10', t)
-        self.assertEqual(len(n.addresses), 485)
+        self.assertEqual(len(n.addresses), 493)
 
     def test_save_snapshot(self):
         t = self._set_receive()
@@ -389,3 +389,12 @@ def test_save_snapshot(self):
         t.save_snapshot()
         s = t.snapshot_set.model.objects.first()
         self.assertFalse(s.created == s.modified)
+
+    def test_issue_58(self):
+        self.node_model.objects.all().delete()
+        self.link_model.objects.all().delete()
+        t = self._set_receive()
+        data = self._load('static/issue-58.json')
+        t.receive(data)
+        self.assertEqual(t.node_set.all().count(), 13)
+        self.assertEqual(t.link_set.all().count(), 11)

django_netjsongraph/tests/static/issue-58.json

@@ -0,0 +1,79 @@
+{
+  "router_id": "Own AS not provided",
+  "version": "",
+  "protocol": "BGP",
+  "links": [
+    {
+      "source": "sv1gsd",
+      "cost": 1.0,
+      "target": "downlots3"
+    },
+    {
+      "source": "sv1gsd",
+      "cost": 1.0,
+      "target": "SV1CIM-SOFIKO-SV1GSD"
+    },
+    {
+      "source": "sofiko_WNK",
+      "cost": 1.0,
+      "target": "Geolos"
+    },
+    {
+      "source": "Kalloni",
+      "cost": 1.0,
+      "target": "Benz_Kalloni_2"
+    },
+    {
+      "source": "sv1gsd",
+      "cost": 1.0,
+      "target": "SV1EGD"
+    },
+    {
+      "source": "sv1gsd_2",
+      "cost": 1.0,
+      "target": "SV1CIM-SOFIKO-SV1GSD"
+    },
+    {
+      "source": "Kalloni",
+      "cost": 1.0,
+      "target": "Metamorfosi"
+    },
+    {
+      "source": "Kalloni",
+      "cost": 1.0,
+      "target": "Benz_Kalloni"
+    },
+    {
+      "source": "sofiko_WNK",
+      "cost": 1.0,
+      "target": "Sv1gyk-Sofiko"
+    },
+    {
+      "source": "Kalloni",
+      "cost": 1.0,
+      "target": "Taktikoupoli"
+    },
+    {
+      "source": "sofiko_WNK",
+      "cost": 1.0,
+      "target": "SV1CIM-SOFIKO-SV1GSD"
+    }
+  ],
+  "metric": "eBGP",
+  "type": "NetworkGraph",
+  "nodes": [
+    {"id": "Sv1gyk-Sofiko"},
+    {"id": "Kalloni"},
+    {"id": "sv1gsd"},
+    {"id": "Benz_Kalloni_2"},
+    {"id": "Metamorfosi"},
+    {"id": "Taktikoupoli"},
+    {"id": "SV1EGD"},
+    {"id": "downlots3"},
+    {"id": "Geolos"},
+    {"id": "SV1CIM-SOFIKO-SV1GSD"},
+    {"id": "sv1gsd_2"},
+    {"id": "sofiko_WNK"},
+    {"id": "Benz_Kalloni"}
+  ]
+}

django_netjsongraph/tests/static/very-long-addresses.json

@@ -6,6 +6,7 @@
     "nodes": [
         {
             "id": "2001:4e12:452a:1:172::10",
+            "label": "very-long-test-node1",
             "local_addresses": [
                 "2001:4e12:452a:1:177::4",
                 "2001:4e12:452a:1:177::3",
@@ -33,7 +34,8 @@
             ]
         },
         {
-            "id": "2001:4e12:452a:1:172::11"
+            "id": "2001:4e12:452a:1:172::11",
+            "label": "very-long-test-node1"
         }
     ],
     "links": [

django_netjsongraph/tests/test_node.py

@@ -8,7 +8,7 @@
 class TestNode(TestNodeMixin, CreateGraphObjectsMixin, TestCase):
     topology_model = Topology
     node_model = Node
-    maxDiff = 0
+    maxDiff = None
 
     def setUp(self):
         t = self._create_topology()