From bb8f5ee12ffc06a685932d1508ac05d537e3c500 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:51 +0200 Subject: [PATCH 1/6] Fix hard related objects not list error The related object fields are not covered by the form, so don't pass any validation before trying to iterate over them and accessing their elements. Instead of allowing a hard technical error to be raised, explicitly check that it is indeed a list, and raise a normal validation error if not. --- netbox/dcim/tests/test_views.py | 36 +++++++++++++++++++++++ netbox/netbox/views/generic/bulk_views.py | 8 ++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 5e41b37f7..822a92818 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -982,6 +982,42 @@ inventory-items: ii1 = InventoryItemTemplate.objects.first() self.assertEqual(ii1.name, 'Inventory Item 1') + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_import_nolist(self): + # 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', + ) + + for value in ('', 'null', '3', '"My console port"', '{name: "My other console port"}'): + with self.subTest(value=value): + import_data = f''' +manufacturer: Manufacturer 1 +model: TEST-2000 +slug: test-2000 +u_height: 1 +console-ports: {value} +''' + 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, "console-ports: Must be a list.") + def test_export_objects(self): url = reverse('dcim:devicetype_list') self.add_permissions('dcim.view_devicetype') diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index b52d12d98..ca26d5a36 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -363,8 +363,14 @@ class BulkImportView(GetReturnURLMixin, BaseMultiObjectView): # Iterate through the related object forms (if any), validating and saving each instance. for field_name, related_object_form in self.related_object_forms.items(): + related_objects = model_form.data.get(field_name, list()) + if not isinstance(related_objects, list): # TODO isinstance(Sequence)? + import_form.add_error(None, f"{field_name}: {_('Must be a list.')}") + raise AbortTransaction() + related_objects = list(enumerate(related_objects)) + related_obj_pks = [] - for i, rel_obj_data in enumerate(model_form.data.get(field_name, list())): + for i, rel_obj_data in related_objects: rel_obj_data = self.prep_related_object_data(obj, rel_obj_data) f = related_object_form(rel_obj_data) From 16e284f03a74895da7dc1f4054045272804a38a5 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:51 +0200 Subject: [PATCH 2/6] Fix hard related object not dict error Elements of the "related objects list" are passed to the `prep_related_object_data` function before any validation takes place, with the potential of failing with a hard error. Similar to the "related objects not list" case explicitly validate the elements general type, and raise a normal validation error if it isn't a dictionary. --- netbox/dcim/tests/test_views.py | 37 +++++++++++++++++++++++ netbox/netbox/views/generic/bulk_views.py | 4 +++ 2 files changed, 41 insertions(+) diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 822a92818..26aad4b68 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1018,6 +1018,43 @@ console-ports: {value} self.assertHttpStatus(response, 200) self.assertContains(response, "console-ports: Must be a list.") + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_import_nodict(self): + # 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', + ) + + for value in ('', 'null', '3', '"My console port"', '["My other console port"]'): + with self.subTest(value=value): + import_data = f''' +manufacturer: Manufacturer 1 +model: TEST-3000 +slug: test-3000 +u_height: 1 +console-ports: + - {value} +''' + 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, "console-ports[0]: Must be a dictionary.") + def test_export_objects(self): url = reverse('dcim:devicetype_list') self.add_permissions('dcim.view_devicetype') diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index ca26d5a36..b89b1d3a0 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -371,6 +371,10 @@ class BulkImportView(GetReturnURLMixin, BaseMultiObjectView): related_obj_pks = [] for i, rel_obj_data in related_objects: + if not isinstance(rel_obj_data, dict): # TODO isinstance(MutableMapping)? + import_form.add_error(None, f"{field_name}[{i}]: {_('Must be a dictionary.')}") + raise AbortTransaction() + rel_obj_data = self.prep_related_object_data(obj, rel_obj_data) f = related_object_form(rel_obj_data) From 18480668e0f5da46ac073314c8a1b4a2e5ac94b2 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:51 +0200 Subject: [PATCH 3/6] Allow bridge when importing interface templates Adding the field to the InterfaceTemplateImportForm is enough. The two clean_*_type methods are for properly dynamically restricting what values can be selected. --- netbox/dcim/forms/object_import.py | 24 +++++++++++++-- netbox/dcim/tests/test_views.py | 49 ++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 3f2cc3ef6..25880348e 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -84,6 +84,12 @@ class InterfaceTemplateImportForm(forms.ModelForm): label=_('Type'), choices=InterfaceTypeChoices.CHOICES ) + bridge = forms.ModelChoiceField( + label=_('Bridge'), + queryset=InterfaceTemplate.objects.all(), + to_field_name='name', + required=False + ) poe_mode = forms.ChoiceField( choices=InterfacePoEModeChoices, required=False, @@ -103,10 +109,24 @@ class InterfaceTemplateImportForm(forms.ModelForm): class Meta: model = InterfaceTemplate fields = [ - 'device_type', 'module_type', 'name', 'label', 'type', 'enabled', 'mgmt_only', 'description', 'poe_mode', - 'poe_type', 'rf_role' + 'device_type', 'module_type', 'name', 'label', 'type', 'enabled', 'mgmt_only', 'description', 'bridge', + 'poe_mode', 'poe_type', 'rf_role' ] + def clean_device_type(self): + if device_type := self.cleaned_data['device_type']: + bridge = self.fields['bridge'] + bridge.queryset = bridge.queryset.filter(device_type=device_type) + + return device_type + + def clean_module_type(self): + if module_type := self.cleaned_data['module_type']: + bridge = self.fields['bridge'] + bridge.queryset = bridge.queryset.filter(module_type=module_type) + + return module_type + class FrontPortTemplateImportForm(forms.ModelForm): type = forms.ChoiceField( diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 26aad4b68..40e3cfef9 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1055,6 +1055,55 @@ console-ports: self.assertHttpStatus(response, 200) self.assertContains(response, "console-ports[0]: Must be a dictionary.") + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_import_interfacebridge(self): + IMPORT_DATA = """ +manufacturer: Manufacturer 1 +model: TEST-4000 +slug: test-4000 +u_height: 1 +interfaces: + - name: Bridge + type: 1000base-t + - name: Bridge Interface 1 + type: 1000base-t + bridge: Bridge +""" + + # 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, "Imported 1 device types") + + device_type = DeviceType.objects.get(model='TEST-4000') + self.assertEqual(device_type.interfacetemplates.count(), 2) + + interfaces = InterfaceTemplate.objects.all().order_by('id') + self.assertEqual(interfaces[0].name, 'Bridge') + self.assertIsNone(interfaces[0].bridge) + self.assertEqual(interfaces[1].name, 'Bridge Interface 1') + self.assertEqual(interfaces[1].bridge.name, "Bridge") + def test_export_objects(self): url = reverse('dcim:devicetype_list') self.add_permissions('dcim.view_devicetype') From fa7b4d8978b0e20e59e6ba478db06a4cd230428c Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:52 +0200 Subject: [PATCH 4/6] Add prep_related_object_list hook Related objects are saved in the same order as they are defined in the input by the user. So related objects with a dependency on another need to be saved after the related object they depend on. Which means that when required, the order needs to be changed. The related object lists are not part of the actual form, and the individual related-object forms only cover the elements themselves. So while the re-order should be done at the form-level during the clean- step, there is no form covering the lists to use for that. Plus should all errors be reported with their original location/index and not with the fixed/resorted one, because these are not useful and even confusing for the user. So the sorting should be done as late as possible, ideally just before the save. This adds a new `prep_related_object_list` hook, which gets passed each list of related objects. This should only be used for modifications to the list (aka order). Any changes to the individual objects should still be done using the existing `prep_related_object_data` hook. The passed list is not the plain list, but an enumerated variant. This is needed to preserve the original index of the elements independently from the actual (possibly changed) order the list, so any later detected errors (for example by the elements form validation) can accurately be reported back to the user. --- netbox/netbox/views/generic/bulk_views.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index b89b1d3a0..b9192b021 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -328,6 +328,13 @@ class BulkImportView(GetReturnURLMixin, BaseMultiObjectView): def get_required_permission(self): return get_permission_for_model(self.queryset.model, 'add') + def prep_related_object_list(self, field_name, enumerated_list): + """ + Hook to modify the enumerated list of related objects before it's passed to the related object form (for + example, to change the order). + """ + pass # TODO keep in-place only, or return modified list? + def prep_related_object_data(self, parent, data): """ Hook to modify the data for related objects before it's passed to the related object form (for example, to @@ -369,6 +376,13 @@ class BulkImportView(GetReturnURLMixin, BaseMultiObjectView): raise AbortTransaction() related_objects = list(enumerate(related_objects)) + try: + self.prep_related_object_list(field_name, related_objects) + except ValidationError as e: + for message in e.messages: + import_form.add_error(None, f"{field_name}: {message}") + raise AbortTransaction() + related_obj_pks = [] for i, rel_obj_data in related_objects: if not isinstance(rel_obj_data, dict): # TODO isinstance(MutableMapping)? From c158bfa87acc9f01c34f9f9cf37f8453021cab23 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:52 +0200 Subject: [PATCH 5/6] 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 From 96cdddd3f8ef0d04b4b5068bcc0bfcb82aa7ef36 Mon Sep 17 00:00:00 2001 From: Marko Hauptvogel Date: Fri, 22 Aug 2025 09:53:52 +0200 Subject: [PATCH 6/6] Add cycle identification Use a depth-first search to identify any cycles, to be able to report them as such to the user. --- netbox/dcim/tests/test_views.py | 68 +++++++++++++++++++++++++++++---- netbox/dcim/views.py | 28 ++++++++++++-- 2 files changed, 84 insertions(+), 12 deletions(-) diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index dcea30cbf..f211e1c88 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1149,15 +1149,24 @@ model: TEST-5000 slug: test-5000 u_height: 1 interfaces: - - name: Interface 1 + - name: Cycle Interface 1 type: 1000base-t - bridge: Interface 2 - - name: Interface 2 + bridge: Cycle Interface 2 + - name: Cycle Interface 2 type: 1000base-t - bridge: Interface 3 - - name: Interface 3 + bridge: Cycle Interface 3 + - name: Cycle Interface 3 type: 1000base-t - bridge: Interface 1 + bridge: Cycle Interface 1 + + - name: Unrelated Interface 1 + type: 1000base-t + - name: Unrelated Interface 2 + type: 1000base-t + bridge: Cycle Interface 1 + - name: Unrelated Interface 3 + type: 1000base-t + bridge: Cycle Interface 3 """ # Add all required permissions to the test user @@ -1183,8 +1192,51 @@ interfaces: 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]") + self.assertContains(response, "interfaces: Dependency cycle [Cycle Interface 1, Cycle Interface 2, " + "Cycle Interface 3, Cycle Interface 1] detected") + + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_import_interfacebridge_invalid(self): + IMPORT_DATA = """ +manufacturer: Manufacturer 1 +model: TEST-6000 +slug: test-6000 +u_height: 1 +interfaces: + - name: Interface 1 + type: 1000base-t + - name: Interface 2 + type: 1000base-t + bridge: Non-existent Bridge + - name: Interface 3 + type: 1000base-t + """ + + # 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[1] bridge: Select a valid choice. " + "That choice is not one of the available choices.") def test_export_objects(self): url = reverse('dcim:devicetype_list') diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index e4c5bc814..0afd46935 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1339,10 +1339,30 @@ class DeviceTypeImportView(generic.BulkImportView): 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)}, - ) + def find_cycle(visited_interfaces): + """Recursive depth-first search to identify cycles.""" + for ifname in list(required_by.get(visited_interfaces[-1], list())): + if ifname in visited_interfaces: + # found a cycle and its start + start_index = visited_interfaces.index(ifname) + visited_interfaces.append(ifname) + return list(reversed(visited_interfaces[start_index:])) + result = find_cycle(visited_interfaces + [ifname]) + if result: + return result + return None + + # Check if there is a cycle + for ifname in unsatisfied_requirements: + cycle = find_cycle([ifname]) + if cycle: + # stop at the first one, finding all while avoiding duplicates would be hard + raise ValidationError( + _("Dependency cycle [%(interfaces)s] detected"), + params={"interfaces": ", ".join(cycle)}, + ) + # no cycle, so the unsatisfied requirements must be due to requirements on non-existent interfaces, + # which will cause a validation error later when checking the individual interface objects. # apply the topological sorting to the actual list def get_sort_key(interface):