Closes #23350: Reduce database queries for reverse many-to-many table columns (#23361)

netbox/ipam/tests/query_counts.json

@@ -2,7 +2,7 @@
   "aggregate:api_list_objects": 13,
   "aggregate:list_objects_with_permission": 21,
   "asn:api_list_objects": 16,
-  "asn:list_objects_with_permission": 28,
+  "asn:list_objects_with_permission": 21,
   "asnrange:api_list_objects": 14,
   "asnrange:list_objects_with_permission": 19,
   "fhrpgroup:api_list_objects": 13,

netbox/ipam/tests/test_tables.py

@@ -1,7 +1,8 @@
-from django.test import TestCase
+from django.test import TestCase, tag
 from netaddr import IPNetwork
 
-from ipam.models import FHRPGroupAssignment, IPAddress, IPRange, Prefix
+from dcim.models import Site
+from ipam.models import ASN, RIR, FHRPGroupAssignment, IPAddress, IPRange, Prefix
 from ipam.tables import *
 from ipam.utils import annotate_ip_space
 from utilities.testing import TableTestCases
@@ -249,6 +250,53 @@ class ASNRangeTableTestCase(TableTestCases.StandardTableTestCase):
 class ASNTableTestCase(TableTestCases.StandardTableTestCase):
     table = ASNTable
 
+    @classmethod
+    def setUpTestData(cls):
+        rir = RIR.objects.create(name='RIR 1', slug='rir-1')
+        asns = (
+            ASN(asn=65001, rir=rir),
+            ASN(asn=65002, rir=rir),
+            ASN(asn=65003, rir=rir),
+        )
+        ASN.objects.bulk_create(asns)
+        cls.sites = (
+            Site(name='Site 1', slug='site-1'),
+            Site(name='Site 2', slug='site-2'),
+        )
+        Site.objects.bulk_create(cls.sites)
+        asns[0].sites.set(cls.sites)
+        asns[1].sites.set(cls.sites[:1])
+
+    @tag('regression')  # Ref: #23350
+    def test_sites_column(self):
+        """The sites column links the sites of each ASN without further queries."""
+        url1, url2 = (site.get_absolute_url() for site in self.sites)
+        table = ASNTable(ASN.objects.all())
+        table.configure(self.get_request())
+        rows = list(table.rows)
+
+        with self.assertNumQueries(0):
+            cells = {row.record.asn: row.get_cell('sites') for row in rows}
+
+        self.assertHTMLEqual(cells[65001], f'<a href="{url1}">Site 1</a>, <a href="{url2}">Site 2</a>')
+        self.assertHTMLEqual(cells[65002], f'<a href="{url1}">Site 1</a>')
+        self.assertEqual(cells[65003], table.columns['sites'].default)
+
+    @tag('regression')  # Ref: #23350
+    def test_sites_export(self):
+        """An export lists the sites of each ASN without further queries."""
+        self.user.config.set('tables.ASNTable.columns', ['asn'], commit=True)
+        table = ASNTable(ASN.objects.all())
+        table.configure(self.get_request())
+        # An "All Data" export prefetches for every column, hidden or not
+        table._apply_prefetching(columns=table.columns.names())
+        rows = list(table.rows)
+
+        with self.assertNumQueries(0):
+            values = {row.record.asn: row.get_cell_value('sites') for row in rows}
+
+        self.assertEqual(values, {65001: 'Site 1, Site 2', 65002: 'Site 1', 65003: None})
+
 
 class ServiceTemplateTableTestCase(TableTestCases.StandardTableTestCase):
     table = ServiceTemplateTable

netbox/netbox/tables/tables.py

@@ -7,7 +7,7 @@ from django.conf import settings
 from django.contrib.contenttypes.fields import GenericForeignKey
 from django.core.exceptions import FieldDoesNotExist
 from django.db.models.fields.related import RelatedField
-from django.db.models.fields.reverse_related import ManyToOneRel
+from django.db.models.fields.reverse_related import ManyToManyRel, ManyToOneRel
 from django.urls.exceptions import NoReverseMatch
 from django.utils.safestring import mark_safe
 from django.utils.translation import gettext_lazy as _
@@ -147,8 +147,8 @@ class BaseTable(tables.Table):
                     field = model._meta.get_field(field_name)
                 except FieldDoesNotExist:
                     break
-                if isinstance(field, (RelatedField, ManyToOneRel)):
-                    # Follow ForeignKeys to the related model
+                if isinstance(field, (RelatedField, ManyToOneRel, ManyToManyRel)):
+                    # Follow relations to the related model
                     prefetch_path.append(field_name)
                     model = field.remote_field.model
                 elif isinstance(field, GenericForeignKey):

netbox/netbox/tests/test_tables.py

@@ -1,6 +1,7 @@
+import django_tables2 as tables
 from django.contrib.auth.models import AnonymousUser
 from django.template import Context, Template
-from django.test import RequestFactory, TestCase, override_settings
+from django.test import RequestFactory, TestCase, override_settings, tag
 from django.utils.html import strip_tags
 
 from core.models import ObjectType
@@ -8,6 +9,8 @@ from dcim.models import Device, Site
 from dcim.tables import DeviceTable
 from extras.choices import CustomFieldChoiceColorChoices, CustomFieldTypeChoices
 from extras.models import CustomField, CustomFieldChoiceSet
+from ipam.models import ASN
+from ipam.tables import ASNTable
 from netbox.tables import NetBoxTable, columns
 from utilities.testing import create_tags, create_test_device, create_test_user
 
@@ -83,6 +86,13 @@ class BaseTableTestCase(TestCase):
         self.assertIn('rack', prefetch_lookups)
         self.assertIn('site__region', prefetch_lookups)
 
+    @tag('regression')  # Ref: #23350
+    def test_prefetch_through_reverse_many_to_many(self):
+        """An accessor through a reverse many-to-many relation is prefetched along its full path."""
+        table = ASNTable(ASN.objects.all(), extra_columns=[('site_regions', tables.Column(accessor='sites__region'))])
+        table._apply_prefetching(columns=['site_regions'])
+        self.assertEqual(table.data.data._prefetch_related_lookups, ('sites__region',))
+
     def test_configure_anonymous_user_with_ordering(self):
         """
         Verify that table.configure() does not raise an error when an anonymous

netbox/users/tests/test_tables.py

@@ -1,5 +1,8 @@
+from django.test import tag
+
+from users.models import Group, ObjectPermission
 from users.tables import *
-from utilities.testing import TableTestCases
+from utilities.testing import TableTestCases, create_test_user
 
 
 class TokenTableTestCase(TableTestCases.StandardTableTestCase):
@@ -17,6 +20,73 @@ class GroupTableTestCase(TableTestCases.StandardTableTestCase):
 class ObjectPermissionTableTestCase(TableTestCases.StandardTableTestCase):
     table = ObjectPermissionTable
 
+    @classmethod
+    def setUpTestData(cls):
+        cls.users = (
+            create_test_user('User 1'),
+            create_test_user('User 2'),
+        )
+        cls.groups = (
+            Group(name='Group 1'),
+            Group(name='Group 2'),
+        )
+        Group.objects.bulk_create(cls.groups)
+        permissions = (
+            ObjectPermission(name='Permission 1', actions=['view']),
+            ObjectPermission(name='Permission 2', actions=['view']),
+            ObjectPermission(name='Permission 3', actions=['view']),
+        )
+        ObjectPermission.objects.bulk_create(permissions)
+        permissions[0].users.set(cls.users)
+        permissions[0].groups.set(cls.groups)
+        permissions[1].users.set(cls.users[:1])
+        permissions[1].groups.set(cls.groups[:1])
+
+    @tag('regression')  # Ref: #23350
+    def test_users_and_groups_columns(self):
+        """The users and groups columns link the assignees of each permission without further queries."""
+        user_url1, user_url2 = (user.get_absolute_url() for user in self.users)
+        group_url1, group_url2 = (group.get_absolute_url() for group in self.groups)
+        self.user.config.set('tables.ObjectPermissionTable.columns', ['name', 'users', 'groups'], commit=True)
+        table = ObjectPermissionTable(ObjectPermission.objects.all())
+        table.configure(self.get_request())
+        rows = list(table.rows)
+
+        with self.assertNumQueries(0):
+            user_cells = {row.record.name: row.get_cell('users') for row in rows}
+            group_cells = {row.record.name: row.get_cell('groups') for row in rows}
+
+        self.assertHTMLEqual(
+            user_cells['Permission 1'],
+            f'<a href="{user_url1}">User 1</a>, <a href="{user_url2}">User 2</a>'
+        )
+        self.assertHTMLEqual(user_cells['Permission 2'], f'<a href="{user_url1}">User 1</a>')
+        self.assertEqual(user_cells['Permission 3'], table.columns['users'].default)
+        self.assertHTMLEqual(
+            group_cells['Permission 1'],
+            f'<a href="{group_url1}">Group 1</a>, <a href="{group_url2}">Group 2</a>'
+        )
+        self.assertHTMLEqual(group_cells['Permission 2'], f'<a href="{group_url1}">Group 1</a>')
+        self.assertEqual(group_cells['Permission 3'], table.columns['groups'].default)
+
+    @tag('regression')  # Ref: #23350
+    def test_users_and_groups_export(self):
+        """An export lists the assignees of each permission without further queries."""
+        table = ObjectPermissionTable(ObjectPermission.objects.all())
+        table.configure(self.get_request())
+        # An "All Data" export prefetches for every column, hidden or not
+        table._apply_prefetching(columns=table.columns.names())
+        rows = list(table.rows)
+
+        with self.assertNumQueries(0):
+            values = {row.record.name: (row.get_cell_value('users'), row.get_cell_value('groups')) for row in rows}
+
+        self.assertEqual(values, {
+            'Permission 1': ('User 1, User 2', 'Group 1, Group 2'),
+            'Permission 2': ('User 1', 'Group 1'),
+            'Permission 3': (None, None),
+        })
+
 
 class OwnerGroupTableTestCase(TableTestCases.StandardTableTestCase):
     table = OwnerGroupTable