Adjust logic for form and serializer. Add tests

netbox/dcim/api/serializers_/device_components.py

@@ -239,25 +239,16 @@ class InterfaceSerializer(NetBoxModelSerializer, CabledObjectSerializer, Connect
             tagged_vlans = []
 
             if self.instance:
-                if 'mode' in data.keys():
+                mode = data.get('mode') if 'mode' in data.keys() else self.instance.mode
-                    mode = data.get('mode') if 'mode' in data.keys() else self.instance.get('mode')
-                if 'tagged_vlans' in data.keys():
                 tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else \
                     self.instance.tagged_vlans.all()
-                if 'untagged_vlan' in data.keys():
-                    untagged_vlan = data.get('untagged_vlan') if 'untagged_vlan' in data.keys() else \
-                        self.instance.untagged_vlan
             else:
-                if 'mode' in data.keys():
+                mode = data.get('mode', None)
-                    mode = data.get('mode')
+                tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else None
-                if 'tagged_vlans' in data.keys():
-                    tagged_vlans = data.get('tagged_vlans')
-                if 'untagged_vlan' in data.keys():
-                    untagged_vlan = data.get('untagged_vlan')
 
             if mode != InterfaceModeChoices.MODE_TAGGED and tagged_vlans:
                 raise serializers.ValidationError({
-                    'tagged_vlans': _("Interface mode does not support including tagged vlans")
+                    'tagged_vlans': _("Interface mode does not support tagged vlans")
                 })
 
             # Validate many-to-many VLAN assignments

netbox/dcim/forms/common.py

@@ -42,7 +42,8 @@ class InterfaceCommonForm(forms.Form):
         super().clean()
 
         parent_field = 'device' if 'device' in self.cleaned_data else 'virtual_machine'
-        tagged_vlans = self.cleaned_data.get('tagged_vlans')
+        interface_mode = get_field_value(self, parent_field)
+        tagged_vlans = get_field_value(self, 'tagged_vlans') if 'tagged_vlans' in self.fields.keys() else []
 
         # Untagged interfaces cannot be assigned tagged VLANs
         if self.cleaned_data['mode'] == InterfaceModeChoices.MODE_ACCESS and tagged_vlans:
@@ -51,8 +52,10 @@ class InterfaceCommonForm(forms.Form):
             })
 
         # Remove all tagged VLAN assignments from "tagged all" interfaces
-        elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED_ALL:
+        elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED_ALL and tagged_vlans:
-            self.cleaned_data['tagged_vlans'] = []
+            raise forms.ValidationError({
+                'mode': _("An tagged-all interface cannot have tagged VLANs assigned.")
+            })
 
         # Validate tagged VLANs; must be a global VLAN or in the same site
         elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED and tagged_vlans:

netbox/dcim/forms/model_forms.py

@@ -1415,37 +1415,6 @@ class InterfaceForm(InterfaceCommonForm, ModularDeviceComponentForm):
             'mode': '802.1Q Mode',
         }
 
-    def clean(self):
-        super().clean()
-
-        # Validate VLAN config
-        mode = None
-        tagged_vlans = []
-        untagged_vlan = None
-
-        if self.instance:
-            if 'mode' in self.cleaned_data.keys():
-                mode = self.cleaned_data.get('mode') if 'mode' in self.cleaned_data.keys() else\
-                    self.instance.get('mode')
-            if 'tagged_vlans' in self.cleaned_data.keys():
-                tagged_vlans = self.cleaned_data.get('tagged_vlans') if 'tagged_vlans' in self.cleaned_data.keys() else\
-                    self.instance.tagged_vlans.all()
-            if 'untagged_vlan' in self.cleaned_data.keys():
-                untagged_vlan = self.cleaned_data.get('untagged_vlan') if 'untagged_vlan' in self.cleaned_data.keys()\
-                    else self.instance.untagged_vlan
-        else:
-            if 'mode' in self.cleaned_data.keys():
-                mode = self.cleaned_data.get('mode')
-            if 'tagged_vlans' in self.cleaned_data.keys():
-                tagged_vlans = self.cleaned_data.get('tagged_vlans')
-            if 'untagged_vlan' in self.cleaned_data.keys():
-                untagged_vlan = self.cleaned_data.get('untagged_vlan')
-
-        if mode != InterfaceModeChoices.MODE_TAGGED and tagged_vlans:
-            raise forms.ValidationError({
-                'tagged_vlans': _("Interface mode does not support including tagged vlans")
-            })
-
 
 class FrontPortForm(ModularDeviceComponentForm):
     rear_port = DynamicModelChoiceField(

netbox/dcim/models/device_components.py

@@ -895,7 +895,7 @@ class Interface(ModularComponentModel, BaseInterface, CabledObjectModel, PathEnd
 
         # VLAN validation
         if not self.mode and self.untagged_vlan:
-            raise ValidationError({'untagged_vlan': _("Interface mode does not support including an untagged vlan.")})
+            raise ValidationError({'untagged_vlan': _("Interface mode does not support an untagged vlan.")})
 
         # Validate untagged VLAN
         if self.untagged_vlan and self.untagged_vlan.site not in [self.device.site, None]:

netbox/dcim/tests/test_api.py

@@ -1,3 +1,5 @@
+import json
+
 from django.test import override_settings
 from django.urls import reverse
 from django.utils.translation import gettext as _
@@ -11,7 +13,7 @@ from ipam.models import ASN, RIR, VLAN, VRF
 from netbox.api.serializers import GenericObjectSerializer
 from tenancy.models import Tenant
 from users.models import User
-from utilities.testing import APITestCase, APIViewTestCases, create_test_device
+from utilities.testing import APITestCase, APIViewTestCases, create_test_device, disable_warnings
 from virtualization.models import Cluster, ClusterType
 from wireless.choices import WirelessChannelChoices
 from wireless.models import WirelessLAN
@@ -1718,6 +1720,32 @@ class InterfaceTest(Mixins.ComponentTraceMixin, APIViewTestCases.APIViewTestCase
         self.client.delete(self._get_list_url(), data, format='json', **self.header)
         self.assertEqual(device.interfaces.count(), 2)  # Child & parent were both deleted
 
+    def test_create_child_interfaces_mode_invalid_data(self):
+        """
+        POST a single object without permission.
+        """
+        self.add_permissions('dcim.add_interface')
+        url = self._get_list_url()
+        initial_count = self._get_queryset().count()
+
+        device = Device.objects.first()
+        vlans = VLAN.objects.all()[0:3]
+
+        create_data = {
+            'device': device.pk,
+            'name': 'Untagged Interface 1',
+            'type': InterfaceTypeChoices.TYPE_1GE_FIXED,
+            'mode': InterfaceModeChoices.MODE_ACCESS,
+            'tagged_vlans': [vlans[0].pk, vlans[1].pk],
+            'untagged_vlan': vlans[2].pk,
+        }
+
+        response = self.client.post(self._get_list_url(), create_data, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        content = json.loads(response.content)
+        self.assertIn('tagged_vlans', content)
+        self.assertIsNone(content.get('data'))
+
 
 class FrontPortTest(APIViewTestCases.APIViewTestCase):
     model = FrontPort

netbox/dcim/tests/test_forms.py

@@ -1,8 +1,9 @@
 from django.test import TestCase
 
-from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices
+from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices, InterfaceModeChoices
 from dcim.forms import *
 from dcim.models import *
+from ipam.models import VLAN
 from utilities.testing import create_test_device
 from virtualization.models import Cluster, ClusterGroup, ClusterType
 
@@ -117,11 +118,23 @@ class DeviceTestCase(TestCase):
         self.assertIn('position', form.errors)
 
 
-class LabelTestCase(TestCase):
+class InterfaceTestCase(TestCase):
 
     @classmethod
     def setUpTestData(cls):
         cls.device = create_test_device('Device 1')
+        cls.vlans = (
+            VLAN(name='VLAN 1', vid=1),
+            VLAN(name='VLAN 2', vid=2),
+            VLAN(name='VLAN 3', vid=3),
+        )
+        VLAN.objects.bulk_create(cls.vlans)
+        cls.interface = Interface.objects.create(
+            device=cls.device,
+            name='Interface 1',
+            type=InterfaceTypeChoices.TYPE_1GE_GBIC,
+            mode=InterfaceModeChoices.MODE_TAGGED,
+        )
 
     def test_interface_label_count_valid(self):
         """
@@ -151,3 +164,129 @@ class LabelTestCase(TestCase):
 
         self.assertFalse(form.is_valid())
         self.assertIn('label', form.errors)
+
+    def test_create_interface_mode_valid_data(self):
+        """
+        Test that saving valid interface mode and tagged/untagged vlans works properly
+        """
+
+        # Validate access mode
+        data = {
+            'device': self.device.pk,
+            'name': 'ethernet1/1',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_ACCESS,
+            'untagged_vlan': self.vlans[0].pk
+        }
+        form = InterfaceCreateForm(data)
+
+        self.assertTrue(form.is_valid())
+
+        # Validate tagged vlans
+        data = {
+            'device': self.device.pk,
+            'name': 'ethernet1/2',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_TAGGED,
+            'untagged_vlan': self.vlans[0].pk,
+            'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk]
+        }
+        form = InterfaceCreateForm(data)
+        self.assertTrue(form.is_valid())
+
+        # Validate tagged vlans
+        data = {
+            'device': self.device.pk,
+            'name': 'ethernet1/3',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
+            'untagged_vlan': self.vlans[0].pk,
+        }
+        form = InterfaceCreateForm(data)
+        self.assertTrue(form.is_valid())
+
+    def test_create_interface_mode_access_invalid_data(self):
+        """
+        Test that saving invalid interface mode and tagged/untagged vlans works properly
+        """
+        data = {
+            'device': self.device.pk,
+            'name': 'ethernet1/4',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_ACCESS,
+            'untagged_vlan': self.vlans[0].pk,
+            'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk]
+        }
+        form = InterfaceCreateForm(data)
+
+        self.assertTrue(form.is_valid())
+
+    def test_edit_interface_mode_access_invalid_data(self):
+        """
+        Test that saving invalid interface mode and tagged/untagged vlans works properly
+        """
+        data = {
+            'device': self.device.pk,
+            'name': 'Ethernet 1/5',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_ACCESS,
+            'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
+        }
+        form = InterfaceForm(data, instance=self.interface)
+
+        self.assertTrue(form.is_valid())
+
+    def test_create_interface_mode_tagged_all_invalid_data(self):
+        """
+        Test that saving invalid interface mode and tagged/untagged vlans works properly
+        """
+        data = {
+            'device': self.device.pk,
+            'name': 'ethernet1/6',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
+            'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2]]
+        }
+        form = InterfaceCreateForm(data)
+
+        self.assertTrue(form.is_valid())
+
+    def test_edit_interface_mode_tagged_all_invalid_data(self):
+        """
+        Test that saving invalid interface mode and tagged/untagged vlans works properly
+        """
+        data = {
+            'device': self.device.pk,
+            'name': 'Ethernet 1/7',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
+            'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2]]
+        }
+        form = InterfaceForm(data, instance=self.interface)
+        form.is_valid()
+        self.assertTrue(form.is_valid())
+
+    def test_edit_interface_mode_tagged_all_existing_invalid_data(self):
+        """
+        Test that saving invalid interface mode and tagged/untagged vlans works properly
+        """
+        self.interface.tagged_vlans.add(self.vlans[0])
+        self.interface.tagged_vlans.add(self.vlans[1])
+        data = {
+            'device': self.device.pk,
+            'name': 'Ethernet 1/8',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
+        }
+        form = InterfaceForm(data, instance=self.interface)
+        self.assertFalse(form.is_valid())
+
+        data = {
+            'device': self.device.pk,
+            'name': 'Ethernet 1/9',
+            'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
+            'mode': InterfaceModeChoices.MODE_ACCESS,
+        }
+        form = InterfaceForm(data, instance=self.interface)
+        self.assertFalse(form.is_valid())
+        self.interface.tagged_vlans.clear()