diff --git a/netbox/netbox/graphql/pagination.py b/netbox/netbox/graphql/pagination.py index 984e41d01..a46a9f73b 100644 --- a/netbox/netbox/graphql/pagination.py +++ b/netbox/netbox/graphql/pagination.py @@ -1,12 +1,15 @@ import strawberry +from django.db import DEFAULT_DB_ALIAS +from django.db.models.functions import DenseRank from strawberry.types.unset import UNSET -from strawberry_django.pagination import _QS, apply +from strawberry_django.pagination import _QS, _PaginationWindow, _resolve_limit, apply from netbox.config import get_config __all__ = ( 'OffsetPaginationInfo', 'OffsetPaginationInput', + 'apply_distinct_window_pagination', 'apply_pagination', ) @@ -26,6 +29,58 @@ class OffsetPaginationInput(OffsetPaginationInfo): pass +def apply_distinct_window_pagination( + queryset: _QS, + *, + related_field_id: str, + offset: int = 0, + limit: int | None = UNSET, +) -> _QS: + """ + Replacement for strawberry-django's `apply_window_pagination()` for a queryset which has `DISTINCT` + enabled, as is the case when a list field is filtered across a to-many relation with `DISTINCT: true`. + + SQL evaluates window functions before `DISTINCT`, so the `ROW_NUMBER()` annotation which + strawberry-django uses to paginate a prefetched relation assigns a unique value to each of the + duplicate rows produced by the join, and `DISTINCT` can never collapse them. `DENSE_RANK()` instead + assigns the same rank to every row which compares equal under the window ordering, leaving the + duplicate rows identical so that `DISTINCT` deduplicates them as intended. And because the rank is + incremented only once per distinct row, the rows are numbered as if the duplicates were never there, + keeping the pagination limit meaningful. + """ + limit = _resolve_limit(limit) + + order_by = [ + expr + for expr, _ in queryset.query.get_compiler( + using=queryset._db or DEFAULT_DB_ALIAS + ).get_order_by() + ] + # Order by the primary key as well, to ensure that two rows representing *different* objects can + # never be assigned the same rank (and hence be counted only once against the limit). + order_by.append('pk') + + # Note that we omit the `_strawberry_total_count` annotation which strawberry-django adds, as it + # cannot be made accurate here: window functions are evaluated before `DISTINCT`, so it would count + # the duplicate rows. strawberry-django's `get_total_count()` already disregards the annotation for + # a queryset with `DISTINCT` enabled and falls back to `count()`, so computing it would be wasted + # work: an extra window aggregate over every joined row. + queryset = queryset.annotate( + _strawberry_row_number=_PaginationWindow( + DenseRank(), + partition_by=related_field_id, + order_by=order_by, + ), + ) + + if offset: + queryset = queryset.filter(_strawberry_row_number__gt=offset) + if limit is not None and limit >= 0: + queryset = queryset.filter(_strawberry_row_number__lte=offset + limit) + + return queryset + + def apply_pagination( self, queryset: _QS, @@ -71,4 +126,14 @@ def apply_pagination( elif pagination.limit <= 0: pagination.limit = max_page_size + # A prefetched relation is paginated with a window function, which is incompatible with the + # `DISTINCT` applied by the filter layer. Fall back to our own implementation in that case. + if pagination is not None and related_field_id is not None and queryset.query.distinct: + return apply_distinct_window_pagination( + queryset, + related_field_id=related_field_id, + offset=pagination.offset, + limit=pagination.limit, + ) + return apply(pagination, queryset, related_field_id=related_field_id) diff --git a/netbox/netbox/tests/test_graphql.py b/netbox/netbox/tests/test_graphql.py index 62d2e4704..92c0af611 100644 --- a/netbox/netbox/tests/test_graphql.py +++ b/netbox/netbox/tests/test_graphql.py @@ -29,6 +29,7 @@ from dcim.models import ( from extras.choices import CustomFieldTypeChoices from extras.models import CustomField, TableConfig, Tag from ipam.models import RIR, Aggregate, IPAddress, Prefix +from netbox.graphql.pagination import apply_distinct_window_pagination from netbox.graphql.scalars import BigInt, BigIntScalar from netbox.graphql.schema import Query, get_schema_extensions, schema from users.models import Token, User @@ -878,6 +879,163 @@ class GraphQLAPITestCase(APITestCase): self.assertNotIn('errors', data) self.assertEqual(len(data['data']['site_list'][0]['devices']), 3) + def test_distinct_nested_list(self): + """ + The `DISTINCT` filter should deduplicate a nested list field which is filtered across a to-many + relation, just as it does for the equivalent top-level list field. + """ + self.add_permissions('dcim.view_device', 'dcim.view_site') + url = reverse('graphql') + + site = Site.objects.get(slug='site-1') + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1') + role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') + devices = Device.objects.bulk_create([ + Device(name=f'Device {i}', site=site, device_type=device_type, role=role) + for i in range(1, 3) + ]) + Interface.objects.bulk_create([ + Interface(device=device, name=f'eth{i}', type='1000base-t') + for device in devices + for i in range(3) + ]) + + # Each device should be returned exactly once, despite having three matching interfaces + query = """ + { + site_list(filters: {slug: {exact: "site-1"}}) { + name + devices(filters: {DISTINCT: true, interfaces: {name: {starts_with: "eth"}}}) { + name + } + } + } + """ + response = self.client.post(url, data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual( + [device['name'] for device in data['data']['site_list'][0]['devices']], + ['Device 1', 'Device 2'] + ) + + # The equivalent top-level query should return the same devices + query = """ + { + device_list(filters: {DISTINCT: true, interfaces: {name: {starts_with: "eth"}}}) { + name + } + } + """ + response = self.client.post(url, data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual( + [device['name'] for device in data['data']['device_list']], + ['Device 1', 'Device 2'] + ) + + @override_settings(MAX_PAGE_SIZE=2) + def test_distinct_nested_list_max_page_size(self): + """ + MAX_PAGE_SIZE should still be enforced on a deduplicated nested list field, and should be applied + to the number of distinct objects returned (not to the number of joined rows). + """ + self.add_permissions('dcim.view_device', 'dcim.view_site') + url = reverse('graphql') + + site = Site.objects.get(slug='site-1') + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1') + role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') + devices = Device.objects.bulk_create([ + Device(name=f'Device {i}', site=site, device_type=device_type, role=role) + for i in range(1, 5) + ]) + Interface.objects.bulk_create([ + Interface(device=device, name=f'eth{i}', type='1000base-t') + for device in devices + for i in range(3) + ]) + + query = """ + { + site_list(filters: {slug: {exact: "site-1"}}) { + name + devices(filters: {DISTINCT: true, interfaces: {name: {starts_with: "eth"}}}) { + name + } + } + } + """ + response = self.client.post(url, data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual( + [device['name'] for device in data['data']['site_list'][0]['devices']], + ['Device 1', 'Device 2'] + ) + + # An explicit offset should likewise be applied to the distinct objects + query = """ + { + site_list(filters: {slug: {exact: "site-1"}}) { + name + devices( + pagination: {offset: 1, limit: 2}, + filters: {DISTINCT: true, interfaces: {name: {starts_with: "eth"}}} + ) { + name + } + } + } + """ + response = self.client.post(url, data={'query': query}, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertNotIn('errors', data) + self.assertEqual( + [device['name'] for device in data['data']['site_list'][0]['devices']], + ['Device 2', 'Device 3'] + ) + + def test_distinct_window_pagination_tied_ordering(self): + """ + Two rows which represent *different* objects must never be assigned the same rank, even when they + compare equal under the queryset's ordering. `DENSE_RANK()` ties such rows by definition, so the + primary key is appended to the window ordering to separate them; without it every device below + would be assigned rank 1 and the limit of two would return all four of them. + """ + site = Site.objects.get(slug='site-1') + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='device-type-1') + role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') + devices = Device.objects.bulk_create([ + Device(name=f'Device {i}', site=site, device_type=device_type, role=role) + for i in range(1, 5) + ]) + Interface.objects.bulk_create([ + Interface(device=device, name=f'eth{i}', type='1000base-t') + for device in devices + for i in range(3) + ]) + + # Order by a column whose value is identical for every device, so that the ordering alone cannot + # distinguish them. Each device additionally matches three interfaces, so the join emits three + # duplicate rows per device which DISTINCT must still collapse. + queryset = Device.objects.filter( + site=site, interfaces__name__startswith='eth' + ).order_by('status').distinct() + queryset = apply_distinct_window_pagination(queryset, related_field_id='site_id', limit=2) + + results = list(queryset) + self.assertEqual(sorted(device.name for device in results), ['Device 1', 'Device 2']) + self.assertEqual(sorted(device._strawberry_row_number for device in results), [1, 2]) + def test_pagination_conflict(self): url = reverse('graphql') query = """