diff --git a/netbox/ipam/managers.py b/netbox/ipam/managers.py index 8f1eb9ff1..e7da0108b 100644 --- a/netbox/ipam/managers.py +++ b/netbox/ipam/managers.py @@ -13,5 +13,9 @@ class IPAddressManager(Manager.from_queryset(IPAddressQuerySet)): address. We can use HOST() to extract just the host portion of the address (ignoring its mask), but we must then re-cast this value to INET() so that records will be ordered properly. We are essentially re-casting each IP address as a /32 or /128. + + Host addresses are not unique, so we must also order by primary key to guarantee a stable, total ordering. + Without this tiebreaker, PostgreSQL is free to return tied rows in a different order from one query to the + next, which causes objects to be duplicated or omitted across paginated requests. """ - return super().get_queryset().order_by(Inet(Host('address'))) + return super().get_queryset().order_by(Inet(Host('address')), 'pk') diff --git a/netbox/ipam/migrations/0094_ipaddress_host_index.py b/netbox/ipam/migrations/0094_ipaddress_host_index.py new file mode 100644 index 000000000..04f0f7c72 --- /dev/null +++ b/netbox/ipam/migrations/0094_ipaddress_host_index.py @@ -0,0 +1,33 @@ +import django.db.models.functions.comparison +from django.db import migrations, models + +import ipam.fields +import ipam.lookups + + +class Migration(migrations.Migration): + dependencies = [ + ('ipam', '0093_alter_prefix__region_alter_prefix__site_group'), + ] + + operations = [ + # Replace the host address index with a composite index which also covers the primary key, so that it can + # satisfy the default ordering of IPAddress outright. Note that the existing index must be dropped rather + # than retained alongside the new one: with both present, PostgreSQL continues to select the narrower index + # and applies an incremental sort atop it. + migrations.RemoveIndex( + model_name='ipaddress', + name='ipam_ipaddress_host', + ), + migrations.AddIndex( + model_name='ipaddress', + index=models.Index( + django.db.models.functions.comparison.Cast( + ipam.lookups.Host('address'), + output_field=ipam.fields.IPAddressField(), + ), + models.F('id'), + name='ipam_ipaddress_host', + ), + ), + ] diff --git a/netbox/ipam/models/ip.py b/netbox/ipam/models/ip.py index 51941526e..231c77c6d 100644 --- a/netbox/ipam/models/ip.py +++ b/netbox/ipam/models/ip.py @@ -1023,8 +1023,13 @@ class IPAddress(ContactsMixin, PrimaryModel): class Meta: ordering = ('address', 'pk') # address may be non-unique indexes = ( - models.Index(fields=('address', 'id')), # Default ordering - models.Index(Cast(Host('address'), output_field=IPAddressField()), name='ipam_ipaddress_host'), + models.Index(fields=('address', 'id')), + # Default ordering (see IPAddressManager). The primary key must be included so that the index can + # satisfy the ordering outright; without it PostgreSQL falls back to an incremental sort, which + # measurably slows deep pagination. + models.Index( + Cast(Host('address'), output_field=IPAddressField()), F('id'), name='ipam_ipaddress_host' + ), models.Index(fields=('assigned_object_type', 'assigned_object_id')), ) verbose_name = _('IP address') diff --git a/netbox/ipam/tests/test_ordering.py b/netbox/ipam/tests/test_ordering.py index 2f904ab89..efd536d14 100644 --- a/netbox/ipam/tests/test_ordering.py +++ b/netbox/ipam/tests/test_ordering.py @@ -198,3 +198,46 @@ class IPAddressOrderingTestCase(OrderingTestBase): # Test self._compare(IPAddress.objects.all(), addresses) + + def test_duplicate_address_ordering(self): + """ + Host addresses are not unique, so tied addresses must be ordered by primary key to yield a stable, total + ordering. Without a tiebreaker the database may return tied rows in a different order from one query to the + next, duplicating or omitting objects across paginated requests. + """ + # Create several duplicates of each address, interleaved so that primary key order does not follow + # address order. + addresses = [ + IPAddress(status=IPAddressStatusChoices.STATUS_ACTIVE, address=netaddr.IPNetwork(f'10.0.{i}.1/24')) + for _ in range(4) + for i in range(100) + ] + IPAddress.objects.bulk_create(addresses) + + pks = list(IPAddress.objects.values_list('pk', flat=True)) + expected = [ + ip.pk for ip in sorted(IPAddress.objects.all(), key=lambda ip: (ip.address.ip, ip.pk)) + ] + + self.assertEqual(pks, expected) + + def test_duplicate_address_pagination(self): + """ + Paginating over duplicate addresses must not return the same object on two pages, nor omit any object. + """ + addresses = [ + IPAddress(status=IPAddressStatusChoices.STATUS_ACTIVE, address=netaddr.IPNetwork(f'10.0.{i}.1/24')) + for _ in range(4) + for i in range(100) + ] + IPAddress.objects.bulk_create(addresses) + + queryset = IPAddress.objects.values_list('pk', flat=True) + page_size = 37 + paginated = [] + for offset in range(0, len(addresses), page_size): + paginated.extend(queryset[offset:offset + page_size]) + + self.assertEqual(len(paginated), len(addresses)) + self.assertEqual(len(set(paginated)), len(addresses)) + self.assertEqual(paginated, list(queryset))