Fixes #22813: Fix extraneous database queries when fetching custom field data via GraphQL API

netbox/extras/graphql/mixins.py

@@ -45,7 +45,7 @@ class ConfigContextMixin:
 @strawberry.type
 class CustomFieldsMixin:
 
-    @strawberry_django.field
+    @strawberry_django.field(only=['custom_field_data'])
     def custom_fields(self) -> strawberry.scalars.JSON:
         return self.custom_field_data
 

netbox/netbox/tests/test_graphql.py

@@ -7,10 +7,12 @@ from django.db import connection
 from django.test import override_settings
 from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
+from django.utils import timezone
 from rest_framework import status
 from strawberry.extensions import QueryDepthLimiter
 from strawberry.schema.config import StrawberryConfig
 
+from core.models import ObjectType
 from dcim.choices import LocationStatusChoices
 from dcim.models import (
     Device,
@@ -23,9 +25,11 @@ from dcim.models import (
     Site,
     VirtualChassis,
 )
-from extras.models import TableConfig, Tag
+from extras.choices import CustomFieldTypeChoices
+from extras.models import CustomField, TableConfig, Tag
 from netbox.graphql.scalars import BigInt, BigIntScalar
 from netbox.graphql.schema import Query, get_schema_extensions, schema
+from users.models import Token
 from utilities.tables import get_table_for_model
 from utilities.testing import APITestCase, APIViewTestCases, TestCase, disable_warnings
 
@@ -638,6 +642,105 @@ class GraphQLAPITestCase(APITestCase):
         self.assertEqual(data['errors'][0]['message'], 'Cannot specify both `start` and `offset` in pagination.')
 
 
+class GraphQLDeferredColumnTestCase(APITestCase):
+    """
+    A GraphQL field backed by a custom resolver is opaque to the query optimizer, which narrows column
+    selection with .only() based on the fields named in the GraphQL document. Any column such a resolver
+    reads must therefore be declared via an `only` hint; otherwise the column is deferred and reading it
+    reloads the row from the database once per object returned (see #22813).
+
+    Each test below asserts both that no single-row reload occurs and that the total query count does not
+    grow with the number of objects returned.
+    """
+    OBJECT_COUNT = 10
+
+    @classmethod
+    def setUpTestData(cls):
+        site = Site.objects.create(name='Site 1', slug='site-1')
+
+        # Devices, for CustomFieldsMixin.custom_fields
+        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')
+        device_role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1')
+        custom_field = CustomField.objects.create(name='cf1', type=CustomFieldTypeChoices.TYPE_TEXT)
+        custom_field.object_types.set([ObjectType.objects.get_for_model(Device)])
+        Device.objects.bulk_create([
+            Device(
+                name=f'Device {i}',
+                device_type=device_type,
+                role=device_role,
+                site=site,
+                custom_field_data={'cf1': f'value {i}'},
+            )
+            for i in range(cls.OBJECT_COUNT)
+        ])
+
+    def _execute(self, query):
+        url = reverse('graphql')
+        with CaptureQueriesContext(connection) as context:
+            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)
+        return data['data'], context.captured_queries
+
+    def assertNoDeferredColumnReloads(self, query_template, list_field, table, validate):
+        """
+        Execute `query_template` (which must accept a `limit` interpolation) for a single object and for
+        OBJECT_COUNT objects, asserting that no row of `table` is re-fetched by primary key and that the
+        total query count is identical for both. `validate` is called with the returned objects.
+        """
+        # Token authentication updates Token.last_used at most once per minute, so the first API request
+        # made by a test issues an additional UPDATE (see netbox/api/authentication.py). Stamp the token
+        # up front to keep that one-off write out of the counts compared below.
+        Token.objects.filter(pk=self.token.pk).update(last_used=timezone.now())
+
+        query_counts = []
+        for limit in (1, self.OBJECT_COUNT):
+            data, queries = self._execute(query_template % {'limit': limit})
+            objects = data[list_field]
+            self.assertEqual(len(objects), limit)
+            validate(objects)
+
+            reloads = [q['sql'] for q in queries if f'FROM "{table}" WHERE "{table}"."id" = ' in q['sql']]
+            self.assertEqual(
+                reloads, [], msg=f'{len(reloads)} deferred-column reload(s) for {limit} object(s): {reloads[:1]}'
+            )
+            query_counts.append(len(queries))
+
+        self.assertEqual(
+            query_counts[0],
+            query_counts[1],
+            msg=(
+                f'Query count grew from {query_counts[0]} to {query_counts[1]} when the number of objects '
+                f'returned grew from 1 to {self.OBJECT_COUNT}'
+            )
+        )
+
+    def test_custom_fields(self):
+        """
+        Regression test for #22813: CustomFieldsMixin.custom_fields must not defer `custom_field_data`.
+        """
+        self.add_permissions('dcim.view_device')
+        query = """
+        {
+            device_list(pagination: {limit: %(limit)s}) {
+                id
+                custom_fields
+            }
+        }
+        """
+
+        expected_values = {f'value {i}' for i in range(self.OBJECT_COUNT)}
+
+        def validate(devices):
+            for device in devices:
+                self.assertEqual(list(device['custom_fields']), ['cf1'])
+                self.assertIn(device['custom_fields']['cf1'], expected_values)
+
+        self.assertNoDeferredColumnReloads(query, 'device_list', 'dcim_device', validate)
+
+
 class GraphQLSchemaCoverageTestCase(APIViewTestCases.GraphQLSchemaCoverageTestCase):
     pass