Fix MAC address pagination by ensuring deterministic ordering

Add 'pk' to MACAddress model ordering to ensure deterministic results
when multiple MAC addresses have the same value. This prevents the same
MAC address from appearing on multiple pages during pagination.

The issue occurred because Django's default ordering by 'mac_address'
alone is non-deterministic when multiple records share the same MAC
address value, causing inconsistent pagination results.

Fixes #19917

Author: jseifeddine

netbox/dcim/migrations/0209_alter_macaddress_options.py

@@ -0,0 +1,15 @@
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('dcim', '0208_devicerole_uniqueness'),
+    ]
+
+    operations = [
+        migrations.AlterModelOptions(
+            name='macaddress',
+            options={'ordering': ('mac_address', 'pk'), 'verbose_name': 'MAC address', 'verbose_name_plural': 'MAC addresses'},
+        ),
+    ] 

netbox/dcim/models/devices.py

@@ -1272,7 +1272,7 @@ class MACAddress(PrimaryModel):
     )
 
     class Meta:
-        ordering = ('mac_address',)
+        ordering = ('mac_address', 'pk',)
         verbose_name = _('MAC address')
         verbose_name_plural = _('MAC addresses')
 

netbox/dcim/tests/test_api.py

@@ -2675,120 +2675,24 @@ def setUpTestData(cls):
             },
         ]
 
-    def test_device_filter_pagination_distinct(self):
-        """
-        Test that filtering MAC addresses by device_id via API returns distinct results
-        across pagination boundaries. Creates many interfaces with the same MAC address
-        to force the pagination duplication bug to manifest.
-        """
-        # Add the required permission for this test
+    def test_mac_address_pagination_duplicates(self):
         self.add_permissions('dcim.view_macaddress')
-        
-        # Create a device with many interfaces all having the same MAC address
-        device = create_test_device('Pagination Test Device')
-        shared_mac_address = '18:2A:D3:65:90:2E'  # Use the same MAC from your original issue
-        
-        # Create many interfaces with the same MAC address to trigger the pagination bug
-        # This simulates the scenario where the same MAC address appears on multiple interfaces
-        # Without .distinct(), the same MAC address object could appear multiple times across pages
-        for i in range(500):  # Create more to really stress test pagination
+        device = create_test_device('Device 2')
+        shared_mac = '00:00:00:00:00:07'
+        for i in range(5):
             interface = Interface.objects.create(
-                device=device,
-                name=f'dummy{i:03d}',
-                type='1000base-t'
+                device=device, name=f'Interface {i:03d}', type='1000base-t'
             )
-            
-            # ALL interfaces get the same MAC address - this triggers the pagination duplication bug
-            MACAddress.objects.create(
-                mac_address=shared_mac_address,
-                assigned_object=interface
-            )
-        
-        # Test API pagination with device_id filter - simulate your script's behavior
+            MACAddress.objects.create(mac_address=shared_mac, assigned_object=interface)
         url = reverse('dcim-api:macaddress-list')
-        expected_count = 500  # Should be exactly 500 unique MAC address objects
-        
-        # Test the exact pagination scenario from your script
-        # Your script uses limit=100, so let's test with that
         all_mac_ids = set()
-        total_collected = 0
-        
-        # Page 1: limit=100 (like your dummy-test.py)
-        response = self.client.get(
-            f'{url}?device_id={device.pk}&limit=100',
-            format='json',
-            **self.header
-        )
-        self.assertHttpStatus(response, status.HTTP_200_OK)
-        self.assertEqual(response.data['count'], expected_count)
-        self.assertEqual(len(response.data['results']), 100)
-        
-        page_1_ids = {result['id'] for result in response.data['results']}
-        all_mac_ids.update(page_1_ids)
-        total_collected += len(response.data['results'])
-        
-        # Continue pagination like your script does
-        next_url = response.data.get('next')
-        page_count = 1
-        
-        while next_url and page_count < 10:  # Limit to prevent infinite loop
-            page_count += 1
-            # Extract just the query parameters from the next URL
-            from urllib.parse import urlparse, parse_qs
-            parsed = urlparse(next_url)
-            query_params = parse_qs(parsed.query)
-            
-            # Build the request
-            params = {}
-            for key, value_list in query_params.items():
-                if value_list:
-                    params[key] = value_list[0]
-            
-            response = self.client.get(url, params, format='json', **self.header)
+        for page in range(1, 6):
+            response = self.client.get(
+                f'{url}?device_id={device.pk}&limit=1&offset={1*(page-1)}',
+                format='json', **self.header
+            )
             self.assertHttpStatus(response, status.HTTP_200_OK)
-            
             page_ids = {result['id'] for result in response.data['results']}
-            
-            # Check for duplicate IDs across pages - this is where the bug would show
-            duplicates = all_mac_ids & page_ids
-            self.assertEqual(len(duplicates), 0, 
-                           f"Found duplicate MAC address IDs between pages: {duplicates}")
-            
+            self.assertEqual(len(all_mac_ids & page_ids), 0)
             all_mac_ids.update(page_ids)
-            total_collected += len(response.data['results'])
-            next_url = response.data.get('next')
-        
-        # The critical test: without .distinct(), total_collected could be > expected_count
-        # because the same MAC address object appears multiple times
-        self.assertEqual(total_collected, expected_count, 
-                        f"Total MAC addresses collected ({total_collected}) should equal expected ({expected_count})")
-        
-        # Verify all collected IDs are unique
-        self.assertEqual(len(all_mac_ids), expected_count, 
-                        f"Should have {expected_count} unique MAC address IDs, got {len(all_mac_ids)}")
-        
-        # Additional verification: all MAC addresses should have the same value
-        # Get all results in one go to verify
-        response_all = self.client.get(
-            f'{url}?device_id={device.pk}&limit=0',  # No pagination limit
-            format='json',
-            **self.header
-        )
-        self.assertHttpStatus(response, status.HTTP_200_OK)
-        
-        # Count interfaces like your original script does
-        interface_mac_count = {}
-        for result in response_all.data['results']:
-            if result['assigned_object']:
-                interface_name = result['assigned_object']['name']
-                if interface_name not in interface_mac_count:
-                    interface_mac_count[interface_name] = []
-                interface_mac_count[interface_name].append(result)
-        
-        # In your scenario, you had 1000 MACs on 997 interfaces due to the bug
-        # Here, we should have exactly 500 MACs on 500 interfaces
-        total_macs_via_interfaces = sum(len(macs) for macs in interface_mac_count.values())
-        self.assertEqual(total_macs_via_interfaces, expected_count, 
-                        f"Total MACs via interface grouping should be {expected_count}")
-        self.assertEqual(len(interface_mac_count), expected_count, 
-                        f"Should have exactly {expected_count} interfaces with MAC addresses")
+        self.assertEqual(len(all_mac_ids), 5)

netbox/dcim/tests/test_filtersets.py

@@ -6941,71 +6941,21 @@ def test_vminterface(self):
         params = {'vminterface': [vm_interfaces[0].name, vm_interfaces[1].name]}
         self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
 
-    def test_device_filter_pagination_distinct(self):
-        """
-        Test that filtering MAC addresses by device_id returns distinct results.
-        Creates many interfaces with the same MAC address to force the pagination
-        duplication bug to manifest across page boundaries.
-        """
-        # Create a device with multiple interfaces
-        device = create_test_device('Test Device')
-        shared_mac_address = '18:2A:D3:65:90:2E'  # Use the same MAC from your original issue
-        
-        # Create 500 interfaces ALL with the same MAC address
-        # This ensures the duplication bug will manifest across pagination boundaries
-        # More interfaces = more likely to trigger pagination duplication
-        for i in range(500):
+    def test_mac_address_pagination_duplicates(self):
+        device = create_test_device('Device 4')
+        shared_mac = '00:00:00:00:00:07'
+        for i in range(5):
             interface = Interface.objects.create(
-                device=device,
-                name=f'dummy{i:03d}',
-                type=InterfaceTypeChoices.TYPE_1GE_FIXED
+                device=device, name=f'Interface {i:03d}', type=InterfaceTypeChoices.TYPE_1GE_FIXED
             )
-            
-            # ALL interfaces get the same MAC address - this forces the pagination bug
-            MACAddress.objects.create(
-                mac_address=shared_mac_address,
-                assigned_object=interface
-            )
-        
-        # Test filtering by device_id
+            MACAddress.objects.create(mac_address=shared_mac, assigned_object=interface)
         params = {'device_id': [device.pk]}
         filtered_queryset = self.filterset(params, self.queryset).qs
-        
-        # Without .distinct(), the bug would cause the same MAC to appear multiple times
-        # With .distinct(), this should return exactly 500 unique MAC address objects
-        expected_count = 500
-        self.assertEqual(filtered_queryset.count(), expected_count)
-        
-        # Verify all MAC addresses are unique objects (no duplicate IDs)
-        mac_ids = list(filtered_queryset.values_list('id', flat=True))
-        self.assertEqual(len(mac_ids), len(set(mac_ids)), "Duplicate MAC address objects found in results")
-        
-        # Verify all returned MAC addresses belong to our test device
-        for mac in filtered_queryset:
-            self.assertEqual(mac.assigned_object.device, device)
-        
-        # Test pagination-like slicing to simulate the bug scenario
-        # This simulates what happens during API pagination with large datasets
-        page_size = 50
-        first_page = filtered_queryset[:page_size]
-        second_page = filtered_queryset[page_size:page_size*2]
-        third_page = filtered_queryset[page_size*2:]
-        
-        # Collect IDs from each page
-        first_page_ids = set(mac.id for mac in first_page)
-        second_page_ids = set(mac.id for mac in second_page)
-        third_page_ids = set(mac.id for mac in third_page)
-        
-        # Ensure no overlap between pages (no duplicate MAC address objects)
-        self.assertEqual(len(first_page_ids & second_page_ids), 0, "Duplicate MAC objects between page 1 and 2")
-        self.assertEqual(len(first_page_ids & third_page_ids), 0, "Duplicate MAC objects between page 1 and 3")
-        self.assertEqual(len(second_page_ids & third_page_ids), 0, "Duplicate MAC objects between page 2 and 3")
-        
-        # Verify total count across all pages
-        all_ids = first_page_ids | second_page_ids | third_page_ids
-        self.assertEqual(len(all_ids), expected_count, "Total count mismatch across pages")
-        
-        # Verify all MACs have the same address but different IDs (different objects)
-        all_mac_addresses = filtered_queryset.values_list('mac_address', flat=True)
-        self.assertTrue(all(addr == shared_mac_address for addr in all_mac_addresses), 
-                       "Not all MAC addresses match the expected shared address")
+        self.assertEqual(filtered_queryset.count(), 5)
+        all_ids = set()
+        for page_start in range(0, 5, 1):
+            page = filtered_queryset[page_start:page_start + 1]
+            page_ids = set(mac.id for mac in page)
+            self.assertEqual(len(all_ids & page_ids), 0)
+            all_ids.update(page_ids)
+        self.assertEqual(len(all_ids), 5)