Address review feedback on NonUniqueModelMultipleChoiceFilter

Require to_field_name, order DISTINCT ON results by pk for a deterministic representative object, extend the filter to virtual machine names, and verify that shared-name filters still match objects on every same-named parent.

netbox/dcim/tests/test_filtersets.py

@@ -62,15 +62,23 @@ class DeviceComponentFilterSetTestMixin:
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
 
     def test_shared_names(self):
-        # A name or slug shared by several objects should resolve to a single filter value
-        location, rack, device = Location.objects.first(), Rack.objects.first(), Device.objects.first()
+        # A name or slug shared by several objects should resolve to a single filter value which still matches
+        # components on each of those objects
+        component = self.queryset.filter(_rack__isnull=False, _location__isnull=False).first()
+        device, rack, location = component.device, component._rack, component._location
+        params = (('device', device.name), ('rack', rack.name), ('location', location.slug))
+        counts = {param: self.filterset({param: [value]}, self.queryset).qs.count() for param, value in params}
+
+        # Move the component to a same-named device, rack, and location in another site
         site = Site.objects.create(name='Site 99', slug='site-99')
-        Location.objects.create(site=site, name=location.name, slug=location.slug)
-        Rack.objects.create(site=site, name=rack.name)
-        Device.objects.create(site=site, name=device.name, device_type=device.device_type, role=device.role)
-        for param, value in (('location', location.slug), ('rack', rack.name), ('device', device.name)):
+        self.queryset.filter(pk=component.pk).update(
+            device=Device.objects.create(site=site, name=device.name, device_type=device.device_type, role=device.role),
+            _rack=Rack.objects.create(site=site, name=rack.name),
+            _location=Location.objects.create(site=site, name=location.name, slug=location.slug),
+        )
+        for param, value in params:
             filterset = self.filterset({param: [value]}, self.queryset)
-            self.assertTrue(filterset.is_valid())
+            self.assertEqual(filterset.qs.count(), counts[param])
             self.assertEqual(len(filterset.form.cleaned_data[param]), 1)
 
 
@@ -9134,15 +9142,17 @@ class MACAddressTestCase(TestCase, ChangeLoggedFilterSetTestMixin):
 
     def test_interface_shared_name(self):
         # A name shared by several interfaces should resolve to a single filter value
-        Interface.objects.create(
+        interface = Interface.objects.create(
             device=Device.objects.get(name='Device 2'), name='Interface 1', type=InterfaceTypeChoices.TYPE_1GE_FIXED
         )
-        VMInterface.objects.create(
+        vm_interface = VMInterface.objects.create(
             virtual_machine=VirtualMachine.objects.get(name='Virtual Machine 2'), name='Interface 1'
         )
+        MACAddress.objects.create(mac_address='00-00-00-01-01-99', assigned_object=interface)
+        MACAddress.objects.create(mac_address='00-00-00-04-01-99', assigned_object=vm_interface)
         for param in ('interface', 'vminterface'):
             filterset = self.filterset({param: ['Interface 1']}, self.queryset)
-            self.assertEqual(filterset.qs.count(), 1)
+            self.assertEqual(filterset.qs.count(), 2)
             self.assertEqual(len(filterset.form.cleaned_data[param]), 1)
 
     def test_assigned(self):

netbox/utilities/filters.py

@@ -182,9 +182,14 @@ class NonUniqueModelMultipleChoiceFilter(django_filters.ModelMultipleChoiceFilte
     """
     A ModelMultipleChoiceFilter for a non-unique to_field_name, which resolves each value to a single object.
     """
+    def __init__(self, *args, **kwargs):
+        if not kwargs.get('to_field_name'):
+            raise ValueError(f"{self.__class__.__name__} requires to_field_name")
+        super().__init__(*args, **kwargs)
+
     def get_queryset(self, request):
         to_field_name = self.extra['to_field_name']
-        return super().get_queryset(request).order_by(to_field_name).distinct(to_field_name)
+        return super().get_queryset(request).order_by(to_field_name, 'pk').distinct(to_field_name)
 
 
 class NullableCharFieldFilter(django_filters.CharFilter):

netbox/utilities/tests/test_filters.py

@@ -37,6 +37,7 @@ from utilities.filters import (
     MultiValueMACAddressFilter,
     MultiValueNumberFilter,
     MultiValueTimeFilter,
+    NonUniqueModelMultipleChoiceFilter,
     TreeNodeMultipleChoiceFilter,
 )
 from wireless.choices import WirelessRoleChoices
@@ -101,6 +102,13 @@ class TreeNodeMultipleChoiceFilterTestCase(TestCase):
         self.assertEqual(qs[1], self.site3)
 
 
+class NonUniqueModelMultipleChoiceFilterTestCase(TestCase):
+
+    def test_to_field_name_required(self):
+        with self.assertRaises(ValueError):
+            NonUniqueModelMultipleChoiceFilter(field_name='device__name', queryset=Device.objects.all())
+
+
 class TagFilterTestCase(TestCase):
     """
     Verify the AND (default), OR (`any`), and NOR (`n`) semantics of TagFilter/TagIDFilter.

netbox/virtualization/filtersets.py

@@ -351,7 +351,7 @@ class VMInterfaceFilterSet(CommonInterfaceFilterSet, OwnerFilterMixin, NetBoxMod
         distinct=False,
         label=_('Virtual machine (ID)'),
     )
-    virtual_machine = django_filters.ModelMultipleChoiceFilter(
+    virtual_machine = NonUniqueModelMultipleChoiceFilter(
         field_name='virtual_machine__name',
         queryset=VirtualMachine.objects.all(),
         distinct=False,
@@ -409,7 +409,7 @@ class VirtualDiskFilterSet(OwnerFilterMixin, NetBoxModelFilterSet):
         distinct=False,
         label=_('Virtual machine (ID)'),
     )
-    virtual_machine = django_filters.ModelMultipleChoiceFilter(
+    virtual_machine = NonUniqueModelMultipleChoiceFilter(
         field_name='virtual_machine__name',
         queryset=VirtualMachine.objects.all(),
         distinct=False,

netbox/virtualization/tests/test_filtersets.py

@@ -855,6 +855,15 @@ class VMInterfaceTestCase(TestCase, ChangeLoggedFilterSetTestMixin):
         params = {'virtual_machine': [vms[0].name, vms[1].name]}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
 
+    def test_virtual_machine_shared_name(self):
+        # A virtual machine name shared by several clusters should resolve to a single filter value
+        vm = VirtualMachine.objects.first()
+        cluster = Cluster.objects.create(name='Cluster 99', type=vm.cluster.type)
+        VirtualMachine.objects.create(cluster=cluster, name=vm.name)
+        filterset = self.filterset({'virtual_machine': [vm.name]}, self.queryset)
+        self.assertTrue(filterset.is_valid())
+        self.assertEqual(len(filterset.form.cleaned_data['virtual_machine']), 1)
+
     def test_mac_address(self):
         params = {'mac_address': ['00-00-00-00-00-01', '00-00-00-00-00-02']}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
@@ -941,6 +950,15 @@ class VirtualDiskTestCase(TestCase, ChangeLoggedFilterSetTestMixin):
         params = {'virtual_machine': [vms[0].name, vms[1].name]}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
 
+    def test_virtual_machine_shared_name(self):
+        # A virtual machine name shared by several clusters should resolve to a single filter value
+        vm = VirtualMachine.objects.first()
+        cluster = Cluster.objects.create(name='Cluster 99', type=vm.cluster.type)
+        VirtualMachine.objects.create(cluster=cluster, name=vm.name)
+        filterset = self.filterset({'virtual_machine': [vm.name]}, self.queryset)
+        self.assertTrue(filterset.is_valid())
+        self.assertEqual(len(filterset.form.cleaned_data['virtual_machine']), 1)
+
     def test_name(self):
         params = {'name': ['Disk 1', 'Disk 2']}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)

netbox/vpn/filtersets.py

@@ -432,7 +432,7 @@ class L2VPNTerminationFilterSet(NetBoxModelFilterSet):
         queryset=Device.objects.all(),
         label=_('Device (ID)'),
     )
-    virtual_machine = django_filters.ModelMultipleChoiceFilter(
+    virtual_machine = NonUniqueModelMultipleChoiceFilter(
         field_name='vminterface__virtual_machine__name',
         queryset=VirtualMachine.objects.all(),
         to_field_name='name',

netbox/vpn/tests/test_filtersets.py

@@ -4,7 +4,7 @@ from dcim.choices import InterfaceTypeChoices
 from dcim.models import Device, Interface, Site
 from ipam.models import VLAN, IPAddress, RouteTarget
 from utilities.testing import ChangeLoggedFilterSetTestMixin, create_test_device, create_test_virtualmachine
-from virtualization.models import VirtualMachine, VMInterface
+from virtualization.models import Cluster, VirtualMachine, VMInterface
 from vpn.choices import *
 from vpn.filtersets import *
 from vpn.models import *
@@ -991,3 +991,12 @@ class L2VPNTerminationTestCase(TestCase, ChangeLoggedFilterSetTestMixin):
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 3)
         params = {'virtual_machine': ['Virtual Machine 1']}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 3)
+
+    def test_virtual_machine_shared_name(self):
+        # A virtual machine name shared by several clusters should resolve to a single filter value
+        vm = VirtualMachine.objects.first()
+        cluster = Cluster.objects.create(name='Cluster 99', type=vm.cluster.type)
+        VirtualMachine.objects.create(cluster=cluster, name=vm.name)
+        filterset = self.filterset({'virtual_machine': [vm.name]}, self.queryset)
+        self.assertTrue(filterset.is_valid())
+        self.assertEqual(len(filterset.form.cleaned_data['virtual_machine']), 1)