From c158bfa87acc9f01c34f9f9cf37f8453021cab23 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:52 +0200 Subject: [PATCH] Sort interface templates of device type Use the new hook to re-sort the interface template list. It is only done when required to preserve the original order and prevent any unneeded computation. But even when sorting, it tries its best to preserve as the original order as much as possible. The sorting is still done before any validation of the individual elements, so needs to be able to work with invalid/incomplete/malformed data. The only guarantee is that we get a list (of something). It builds a simple dependency graph, uses Kahn's algorithm to create a topological sorting of it, and applies that to the interface template list. --- netbox/dcim/tests/test_views.py | 92 +++++++++++++++++++++++++++++++-- netbox/dcim/views.py | 77 ++++++++++++++++++++++++++- 2 files changed, 163 insertions(+), 6 deletions(-) diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 40e3cfef9..dcea30cbf 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1063,11 +1063,32 @@ model: TEST-4000 slug: test-4000 u_height: 1 interfaces: - - name: Bridge + - name: Standalone 1 type: 1000base-t + - name: Bridge Interface 2 + type: 1000base-t + bridge: Bridge - name: Bridge Interface 1 type: 1000base-t bridge: Bridge + - name: Standalone 2 + type: 1000base-t + - name: Sub-Bridge Interface 2 + type: 1000base-t + bridge: Sub-Bridge + - name: Bridge + type: 1000base-t + - name: Sub-Bridge + type: 1000base-t + bridge: Bridge + - name: Bridge Interface 3 + type: 1000base-t + bridge: Bridge + - name: Sub-Bridge Interface 1 + type: 1000base-t + bridge: Sub-Bridge + - name: Standalone 3 + type: 1000base-t """ # Add all required permissions to the test user @@ -1096,13 +1117,74 @@ interfaces: self.assertContains(response, "Imported 1 device types") device_type = DeviceType.objects.get(model='TEST-4000') - self.assertEqual(device_type.interfacetemplates.count(), 2) + self.assertEqual(device_type.interfacetemplates.count(), 10) interfaces = InterfaceTemplate.objects.all().order_by('id') - self.assertEqual(interfaces[0].name, 'Bridge') + self.assertEqual(interfaces[0].name, 'Standalone 1') self.assertIsNone(interfaces[0].bridge) - self.assertEqual(interfaces[1].name, 'Bridge Interface 1') - self.assertEqual(interfaces[1].bridge.name, "Bridge") + self.assertEqual(interfaces[1].name, 'Standalone 2') + self.assertIsNone(interfaces[1].bridge) + self.assertEqual(interfaces[2].name, 'Bridge') + self.assertIsNone(interfaces[2].bridge) + self.assertEqual(interfaces[3].name, 'Standalone 3') + self.assertIsNone(interfaces[3].bridge) + self.assertEqual(interfaces[4].name, 'Bridge Interface 2') + self.assertEqual(interfaces[4].bridge.name, "Bridge") + self.assertEqual(interfaces[5].name, 'Bridge Interface 1') + self.assertEqual(interfaces[5].bridge.name, "Bridge") + self.assertEqual(interfaces[6].name, 'Sub-Bridge') + self.assertEqual(interfaces[6].bridge.name, "Bridge") + self.assertEqual(interfaces[7].name, 'Bridge Interface 3') + self.assertEqual(interfaces[7].bridge.name, "Bridge") + self.assertEqual(interfaces[8].name, 'Sub-Bridge Interface 2') + self.assertEqual(interfaces[8].bridge.name, "Sub-Bridge") + self.assertEqual(interfaces[9].name, 'Sub-Bridge Interface 1') + self.assertEqual(interfaces[9].bridge.name, "Sub-Bridge") + + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_import_interfacebridge_cycle(self): + IMPORT_DATA = """ +manufacturer: Manufacturer 1 +model: TEST-5000 +slug: test-5000 +u_height: 1 +interfaces: + - name: Interface 1 + type: 1000base-t + bridge: Interface 2 + - name: Interface 2 + type: 1000base-t + bridge: Interface 3 + - name: Interface 3 + type: 1000base-t + bridge: Interface 1 + """ + + # Add all required permissions to the test user + self.add_permissions( + 'dcim.view_devicetype', + 'dcim.add_devicetype', + 'dcim.add_consoleporttemplate', + 'dcim.add_consoleserverporttemplate', + 'dcim.add_powerporttemplate', + 'dcim.add_poweroutlettemplate', + 'dcim.add_interfacetemplate', + 'dcim.add_frontporttemplate', + 'dcim.add_rearporttemplate', + 'dcim.add_modulebaytemplate', + 'dcim.add_devicebaytemplate', + 'dcim.add_inventoryitemtemplate', + ) + + form_data = { + 'data': IMPORT_DATA, + 'format': 'yaml' + } + + response = self.client.post(reverse('dcim:devicetype_bulk_import'), data=form_data, follow=True) + self.assertHttpStatus(response, 200) + self.assertContains(response, "interfaces: Dependency cycle detected in subset " + "[Interface 1, Interface 2, Interface 3]") def test_export_objects(self): url = reverse('dcim:devicetype_list') diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 304438698..e4c5bc814 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1,9 +1,11 @@ +from collections import deque + from django.contrib import messages from django.contrib.contenttypes.models import ContentType from django.core.paginator import EmptyPage, PageNotAnInteger from django.db import router, transaction from django.db.models import Prefetch -from django.forms import ModelMultipleChoiceField, MultipleHiddenInput, modelformset_factory +from django.forms import ModelMultipleChoiceField, MultipleHiddenInput, ValidationError, modelformset_factory from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.html import escape @@ -1283,6 +1285,79 @@ class DeviceTypeImportView(generic.BulkImportView): 'inventory-items': forms.InventoryItemTemplateImportForm, } + def _sort_interfaces(self, interfaces): + """Sort the given enumerated interface list to satisfy any dependencies.""" + if not interfaces: + return + + # build the dependency graph + all_interface_names = set() + sorting_required = False + required_by = dict() # ifname to list of dependant subinterfaces # TODO replace list with ordered set + requires = dict() # ifname to set of depended-on superinterfaces + for idx, interface in interfaces: + if not isinstance(interface, dict): # TODO isinstance(MutableMapping)? + # not a dict, will fail validation anyway + # but still sort if required, to prevent false "bridge is invalid" errors on any other valid interfaces + continue + if not interface.get('name'): + # no interface name, will fail validation anyway + continue + + ifname = interface['name'] + all_interface_names.add(ifname) + + requirements = set() + if bridge := interface.get('bridge'): + requirements.add(bridge) + + requires[ifname] = requirements + for requirement in list(requirements): + required_by.setdefault(requirement, list()).append(ifname) + + # if we haven't seen all requirements yet, sorting is needed + sorting_required |= not requirements.issubset(all_interface_names) + + if not sorting_required: + return + + # use Kahn's algorithm to build a topological sorting + workqueue = deque(ifname for ifname, requirements in requires.items() if not requirements) + ifname_ordering = list() + while workqueue: + ifname = workqueue.popleft() + ifname_ordering.append(ifname) + + for dependant in list(required_by.get(ifname, list())): + requires[dependant].remove(ifname) + if not requires[dependant]: + workqueue.append(dependant) + + if len(ifname_ordering) != len(set(ifname_ordering)): + # should never happen + raise ValueError("Interface ordering contains duplicates") + + unsatisfied_requirements = list(ifname for ifname, deps in requires.items() if deps) + if unsatisfied_requirements: + raise ValidationError( + _("Dependency cycle detected in subset [%(interfaces)s]"), + params={"interfaces": ", ".join(unsatisfied_requirements)}, + ) + + # apply the topological sorting to the actual list + def get_sort_key(interface): + try: + return ifname_ordering.index(interface['name']) + except Exception: + # Everything broken/invalid goes to the beginning, so validation fails fast + return -1 + + interfaces.sort(key=lambda entry_tuple: get_sort_key(entry_tuple[1])) + + def prep_related_object_list(self, field_name, enumerated_list): + if field_name == 'interfaces': + self._sort_interfaces(enumerated_list) + def prep_related_object_data(self, parent, data): data.update({'device_type': parent}) return data