Jelajahi Sumber

perf(api): Skip config context annotation when field is omitted

List and detail responses which omit config_context now defer the cache
and skip annotation, avoiding overhead when the field isn't requested.
Export templates and bulk updates still load the cache to prevent N+1
queries in full_clean() and change logging.

Fixes #23288
Martin Hauser 3 hari lalu
induk
melakukan
61b6d6dae8

+ 110 - 1
netbox/dcim/tests/test_api.py

@@ -20,7 +20,7 @@ from dcim.choices import *
 from dcim.constants import *
 from dcim.constants import *
 from dcim.graphql.types import _CABLE_TERMINATION_MODELS
 from dcim.graphql.types import _CABLE_TERMINATION_MODELS
 from dcim.models import *
 from dcim.models import *
-from extras.models import ConfigTemplate, Tag
+from extras.models import ConfigContext, ConfigTemplate, ExportTemplate, Tag
 from ipam.choices import VLANQinQRoleChoices
 from ipam.choices import VLANQinQRoleChoices
 from ipam.models import ASN, RIR, VLAN, VRF, IPAddress
 from ipam.models import ASN, RIR, VLAN, VRF, IPAddress
 from netbox.api.serializers import GenericObjectSerializer
 from netbox.api.serializers import GenericObjectSerializer
@@ -2662,6 +2662,115 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase):
 
 
         self.assertEqual(response.data['results'][0].get('config_context', {}).get('A'), 1)
         self.assertEqual(response.data['results'][0].get('config_context', {}).get('A'), 1)
 
 
+    def test_config_context_not_loaded_when_omitted(self):
+        """
+        List and detail responses which omit config_context neither annotate it nor load the cached context.
+        """
+        self.add_permissions('dcim.view_device')
+        device = Device.objects.get(name='Device 1')
+        cases = (
+            (self._get_list_url(), {'fields': 'id,name'}),
+            (self._get_list_url(), {'omit': 'config_context'}),
+            (self._get_list_url(), {'brief': 1}),
+            (self._get_detail_url(device), {'fields': 'id,name'}),
+        )
+        for url, params in cases:
+            with self.subTest(url=url, params=params):
+                with CaptureQueriesContext(connection) as queries:
+                    response = self.client.get(url, params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                sql = '\n'.join(query['sql'] for query in queries.captured_queries)
+                self.assertNotIn('"config_context_data"', sql)
+                self.assertNotIn('"dcim_device"."_config_context_data"', sql)
+
+    def test_config_context_loaded_when_requested(self):
+        """
+        Responses which include config_context read warm devices from the cache and cold devices from the
+        annotation, without a query per device.
+        """
+        self.add_permissions('dcim.view_device')
+        ConfigContext.objects.create(name='Config Context 1', weight=100, data={'foo': 123})
+        Device.objects.filter(name='Device 1').update(_config_context_data={'foo': 'cached'})
+        # fields takes precedence over omit, and both over brief
+        for params in (
+            {},
+            {'fields': 'name,config_context'},
+            {'fields': 'name,config_context', 'omit': 'config_context'},
+            {'brief': 1, 'fields': 'name,config_context'},
+            {'brief': 1, 'omit': 'comments'},
+        ):
+            with self.subTest(params=params):
+                with CaptureQueriesContext(connection) as queries:
+                    response = self.client.get(self._get_list_url(), params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                contexts = {row['name']: row['config_context'] for row in response.data['results']}
+                self.assertEqual(contexts, {
+                    'Device 1': {'foo': 'cached'},
+                    'Device 2': {'foo': 123, 'B': 2},
+                    'Device 3': {'foo': 123, 'C': 3},
+                })
+                statements = [
+                    query['sql'] for query in queries.captured_queries if '"extras_configcontext"' in query['sql']
+                ]
+                self.assertEqual(len(statements), 1)
+                self.assertTrue(statements[0].startswith('SELECT "dcim_device"'))
+                refreshes = [
+                    query['sql'] for query in queries.captured_queries
+                    if query['sql'].startswith('SELECT "dcim_device"."id", "dcim_device"."_config_context_data" FROM')
+                ]
+                self.assertEqual(refreshes, [])
+
+    def test_export_template_loads_config_context(self):
+        """
+        An API export renders model instances, so it loads config context even when the request omits the field.
+        """
+        self.add_permissions('dcim.view_device', 'extras.view_exporttemplate')
+        ConfigContext.objects.create(name='Config Context 1', weight=100, data={'foo': 123})
+        Device.objects.filter(name='Device 1').update(_config_context_data={'foo': 'cached'})
+        export_template = ExportTemplate.objects.create(
+            name='Config Contexts',
+            template_code=(
+                '{% for device in queryset %}{{ device.name }}={{ device.get_config_context().foo }},{% endfor %}'
+            ),
+        )
+        export_template.object_types.set([ObjectType.objects.get_for_model(Device)])
+
+        with CaptureQueriesContext(connection) as queries:
+            response = self.client.get(
+                self._get_list_url(), {'export': export_template.name, 'omit': 'config_context'}, **self.header
+            )
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(response.content.decode(), 'Device 1=cached,Device 2=123,Device 3=123,')
+        statements = [
+            query['sql'] for query in queries.captured_queries if '"extras_configcontext"' in query['sql']
+        ]
+        self.assertEqual(len(statements), 1)
+        self.assertTrue(statements[0].startswith('SELECT "dcim_device"'))
+        refreshes = [
+            query['sql'] for query in queries.captured_queries
+            if query['sql'].startswith('SELECT "dcim_device"."id", "dcim_device"."_config_context_data" FROM')
+        ]
+        self.assertEqual(refreshes, [])
+
+    def test_bulk_update_keeps_config_context_cache_loaded(self):
+        """
+        A bulk update which omits config_context loads the cache with the devices, not with a query per device.
+        """
+        self.add_permissions('dcim.change_device')
+        data = [{'id': device.pk, 'description': 'New description'} for device in Device.objects.all()]
+
+        with CaptureQueriesContext(connection) as queries:
+            response = self.client.patch(
+                f'{self._get_list_url()}?omit=config_context', data, format='json', **self.header
+            )
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        # The change log snapshot and full_clean() would fetch a deferred cache once per device
+        refreshes = [
+            query['sql'] for query in queries.captured_queries
+            if query['sql'].startswith('SELECT "dcim_device"."id", "dcim_device"."_config_context_data" FROM')
+        ]
+        self.assertEqual(refreshes, [])
+
     def test_unique_name_per_site_constraint(self):
     def test_unique_name_per_site_constraint(self):
         """
         """
         Check that creating a device with a duplicate name within a site fails.
         Check that creating a device with a duplicate name within a site fails.

+ 13 - 4
netbox/extras/api/mixins.py

@@ -32,21 +32,30 @@ class ConfigContextQuerySetMixin:
     """
     """
     Used by viewsets for config context models (Device, VirtualMachine).
     Used by viewsets for config context models (Device, VirtualMachine).
 
 
-    For non-brief requests, annotates the queryset so that config context data is computed in a
+    Annotates the queryset so that config context data is computed in a
     single query for any object whose pre-rendered cache (`_config_context_data`) has been
     single query for any object whose pre-rendered cache (`_config_context_data`) has been
     invalidated (NULL). Objects with a warm cache are served directly from it by
     invalidated (NULL). Objects with a warm cache are served directly from it by
     ConfigContextModel.get_config_context() and incur no subquery — PostgreSQL short-circuits the
     ConfigContextModel.get_config_context() and incur no subquery — PostgreSQL short-circuits the
     CASE, so the correlated aggregation runs only for the invalidated rows. This avoids the
     CASE, so the correlated aggregation runs only for the invalidated rows. This avoids the
     per-object fallback query that would otherwise occur when listing objects with cold caches
     per-object fallback query that would otherwise occur when listing objects with cold caches
     (e.g. immediately following an upgrade or a broad invalidation).
     (e.g. immediately following an upgrade or a broad invalidation).
+
+    List and detail responses which omit `config_context` skip the annotation and defer the cache.
     """
     """
     def get_queryset(self):
     def get_queryset(self):
         queryset = super().get_queryset()
         queryset = super().get_queryset()
-        # Brief responses omit config_context entirely, so the annotation would be pure overhead.
-        if self.brief:
-            return queryset
+        if self._config_context_omitted():
+            return queryset.defer('_config_context_data')
         return queryset.annotate_config_context_data(only_invalidated=True)
         return queryset.annotate_config_context_data(only_invalidated=True)
 
 
+    def _config_context_omitted(self):
+        # Writes, custom actions and export templates read the cache outside the serializer
+        if self.action not in ('list', 'retrieve') or 'export' in self.request.query_params:
+            return False
+        fields = set(self.field_kwargs.get('fields') or self.get_serializer_class().Meta.fields)
+        fields.difference_update(self.field_kwargs.get('omit') or ())
+        return 'config_context' not in fields
+
 
 
 class ConfigTemplateRenderMixin:
 class ConfigTemplateRenderMixin:
     """
     """

+ 57 - 1
netbox/virtualization/tests/test_api.py

@@ -1,8 +1,10 @@
 import logging
 import logging
 from decimal import Decimal
 from decimal import Decimal
 
 
+from django.db import connection
 from django.db.models.signals import post_init
 from django.db.models.signals import post_init
 from django.test import override_settings, tag
 from django.test import override_settings, tag
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
 from django.urls import reverse
 from netaddr import IPNetwork
 from netaddr import IPNetwork
 from rest_framework import status
 from rest_framework import status
@@ -11,7 +13,7 @@ from core.models import ObjectType
 from dcim.choices import InterfaceModeChoices
 from dcim.choices import InterfaceModeChoices
 from dcim.models import Platform, Site
 from dcim.models import Platform, Site
 from extras.choices import CustomFieldTypeChoices
 from extras.choices import CustomFieldTypeChoices
-from extras.models import ConfigTemplate, CustomField
+from extras.models import ConfigContext, ConfigTemplate, CustomField
 from ipam.choices import VLANQinQRoleChoices
 from ipam.choices import VLANQinQRoleChoices
 from ipam.models import VLAN, VRF, IPAddress, Prefix
 from ipam.models import VLAN, VRF, IPAddress, Prefix
 from users.constants import TOKEN_PREFIX
 from users.constants import TOKEN_PREFIX
@@ -468,6 +470,60 @@ class VirtualMachineTestCase(APIViewTestCases.APIViewTestCase):
         response = self.client.get(url, **self.header)
         response = self.client.get(url, **self.header)
         self.assertEqual(response.data['results'][0].get('config_context', {}).get('A'), 1)
         self.assertEqual(response.data['results'][0].get('config_context', {}).get('A'), 1)
 
 
+    def test_config_context_not_loaded_when_omitted(self):
+        """
+        List responses which omit config_context neither annotate it nor load the cached context.
+        """
+        self.add_permissions('virtualization.view_virtualmachine')
+        for params in ({'fields': 'id,name'}, {'omit': 'config_context'}, {'brief': 1}):
+            with self.subTest(params=params):
+                with CaptureQueriesContext(connection) as queries:
+                    response = self.client.get(self._get_list_url(), params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                sql = '\n'.join(query['sql'] for query in queries.captured_queries)
+                self.assertNotIn('"config_context_data"', sql)
+                # The nested device still loads its own cache
+                self.assertNotIn('"virtualization_virtualmachine"."_config_context_data"', sql)
+
+    def test_config_context_loaded_when_requested(self):
+        """
+        Responses which include config_context read warm virtual machines from the cache and cold ones from the
+        annotation, without a query per virtual machine.
+        """
+        self.add_permissions('virtualization.view_virtualmachine')
+        ConfigContext.objects.create(name='Config Context 1', weight=100, data={'foo': 123})
+        VirtualMachine.objects.filter(name='Virtual Machine 1').update(_config_context_data={'foo': 'cached'})
+        cache_refresh = (
+            'SELECT "virtualization_virtualmachine"."id", "virtualization_virtualmachine"."_config_context_data" FROM'
+        )
+        # fields takes precedence over omit, and both over brief
+        for params in (
+            {},
+            {'fields': 'name,config_context'},
+            {'fields': 'name,config_context', 'omit': 'config_context'},
+            {'brief': 1, 'fields': 'name,config_context'},
+            {'brief': 1, 'omit': 'comments'},
+        ):
+            with self.subTest(params=params):
+                with CaptureQueriesContext(connection) as queries:
+                    response = self.client.get(self._get_list_url(), params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                contexts = {row['name']: row['config_context'] for row in response.data['results']}
+                self.assertEqual(contexts, {
+                    'Virtual Machine 1': {'foo': 'cached'},
+                    'Virtual Machine 2': {'foo': 123, 'B': 2},
+                    'Virtual Machine 3': {'foo': 123, 'C': 3},
+                })
+                statements = [
+                    query['sql'] for query in queries.captured_queries if '"extras_configcontext"' in query['sql']
+                ]
+                self.assertEqual(len(statements), 1)
+                self.assertTrue(statements[0].startswith('SELECT "virtualization_virtualmachine"'))
+                refreshes = [
+                    query['sql'] for query in queries.captured_queries if query['sql'].startswith(cache_refresh)
+                ]
+                self.assertEqual(refreshes, [])
+
     def test_unique_name_per_cluster_constraint(self):
     def test_unique_name_per_cluster_constraint(self):
         """
         """
         Check that creating a virtual machine with a duplicate name fails.
         Check that creating a virtual machine with a duplicate name fails.