diff --git a/django_netjsongraph/base/link.py b/django_netjsongraph/base/link.py index f48158e..08225e4 100644 --- a/django_netjsongraph/base/link.py +++ b/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, diff --git a/django_netjsongraph/base/node.py b/django_netjsongraph/base/node.py index 5d5c617..60740d7 100644 --- a/django_netjsongraph/base/node.py +++ b/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() diff --git a/django_netjsongraph/migrations/0006_reformat_addresses.py b/django_netjsongraph/migrations/0006_reformat_addresses.py new file mode 100644 index 0000000..6aee656 --- /dev/null +++ b/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), + ] diff --git a/django_netjsongraph/tests/base/test_node.py b/django_netjsongraph/tests/base/test_node.py index f618f1a..38f9154 100644 --- a/django_netjsongraph/tests/base/test_node.py +++ b/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,6 +82,7 @@ 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) @@ -87,6 +90,16 @@ def test_get_from_address(self): 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) diff --git a/django_netjsongraph/tests/base/test_topology.py b/django_netjsongraph/tests/base/test_topology.py index c12122a..751397f 100644 --- a/django_netjsongraph/tests/base/test_topology.py +++ b/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) diff --git a/django_netjsongraph/tests/static/issue-58.json b/django_netjsongraph/tests/static/issue-58.json new file mode 100644 index 0000000..baba2c9 --- /dev/null +++ b/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"} + ] +} diff --git a/django_netjsongraph/tests/static/very-long-addresses.json b/django_netjsongraph/tests/static/very-long-addresses.json index 7bfdc26..afe94fc 100644 --- a/django_netjsongraph/tests/static/very-long-addresses.json +++ b/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": [ diff --git a/django_netjsongraph/tests/test_node.py b/django_netjsongraph/tests/test_node.py index 123b8ec..fae3e2a 100644 --- a/django_netjsongraph/tests/test_node.py +++ b/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()