Quellcode durchsuchen

Closes #22090: Extend test cases to analyze the number of SQL queries executed

Jeremy Stretch vor 1 Tag
Ursprung
Commit
07c2c79450

+ 12 - 0
docs/development/getting-started.md

@@ -186,6 +186,18 @@ This is handy for instances where just a few tests are failing and you want to r
 !!! info
     NetBox uses [django-rich](https://github.com/adamchainz/django-rich) to enhance Django's default `test` management command.
 
+### SQL Query Count Baselines
+
+The shared list-test mixins assert the number of SQL queries each list endpoint performs against a baseline checked in alongside the tests. This guards against the accidental introduction of new queries (e.g. N+1 patterns) when a queryset, serializer, or table changes. Baselines are stored per app at `netbox/<app>/tests/query_counts.json`, keyed by `<model_name>:<test_name>`.
+
+If a list test fails with a message like `Query count for dcim/site:list_objects changed: expected 16, got 18`, first investigate whether the change is expected. If the new count is correct (e.g. you intentionally added a `prefetch_related`, or removed one), regenerate the baseline:
+
+```no-highlight
+UPDATE_QUERY_COUNTS=1 python manage.py test --keepdb
+```
+
+`UPDATE_QUERY_COUNTS` mode requires serial execution; do not combine it with `--parallel`. You can target a single test, app, or the full suite — only the keys exercised by the run are updated. Review the resulting diff in the JSON files as part of the PR; a reviewer should be able to see and reason about every query-count change.
+
 ## Submitting Pull Requests
 
 Once you're happy with your work and have verified that all tests pass, commit your changes and push it upstream to your fork. Always provide descriptive (but not excessively verbose) commit messages. Be sure to prefix your commit message with the word "Fixes" or "Closes" and the relevant issue number (with a hash mark). This tells GitHub to automatically close the referenced issue once the commit has been merged.

+ 24 - 0
netbox/circuits/tests/query_counts.json

@@ -0,0 +1,24 @@
+{
+  "circuit:api_list_objects": 16,
+  "circuit:list_objects_with_permission": 22,
+  "circuitgroup:api_list_objects": 13,
+  "circuitgroup:list_objects_with_permission": 20,
+  "circuitgroupassignment:api_list_objects": 17,
+  "circuitgroupassignment:list_objects_with_permission": 26,
+  "circuittermination:api_list_objects": 18,
+  "circuittermination:list_objects_with_permission": 24,
+  "circuittype:api_list_objects": 13,
+  "circuittype:list_objects_with_permission": 20,
+  "provider:api_list_objects": 15,
+  "provider:list_objects_with_permission": 20,
+  "provideraccount:api_list_objects": 14,
+  "provideraccount:list_objects_with_permission": 21,
+  "providernetwork:api_list_objects": 14,
+  "providernetwork:list_objects_with_permission": 21,
+  "virtualcircuit:api_list_objects": 16,
+  "virtualcircuit:list_objects_with_permission": 24,
+  "virtualcircuittermination:api_list_objects": 17,
+  "virtualcircuittermination:list_objects_with_permission": 23,
+  "virtualcircuittype:api_list_objects": 13,
+  "virtualcircuittype:list_objects_with_permission": 20
+}

+ 8 - 0
netbox/core/tests/query_counts.json

@@ -0,0 +1,8 @@
+{
+  "datafile:api_list_objects": 10,
+  "datafile:list_objects_with_permission": 18,
+  "datasource:api_list_objects": 12,
+  "datasource:list_objects_with_permission": 20,
+  "job:api_list_objects": 12,
+  "job:list_objects_with_permission": 19
+}

+ 83 - 0
netbox/dcim/tests/query_counts.json

@@ -0,0 +1,83 @@
+{
+  "cable:api_list_objects": 24,
+  "cable:list_objects_with_permission": 24,
+  "cablebundle:api_list_objects": 13,
+  "cablebundle:list_objects_with_permission": 20,
+  "cabletermination:api_list_objects": 16,
+  "consoleport:api_list_objects": 14,
+  "consoleport:list_objects_with_permission": 21,
+  "consoleporttemplate:api_list_objects": 11,
+  "consoleserverport:api_list_objects": 14,
+  "consoleserverport:list_objects_with_permission": 21,
+  "consoleserverporttemplate:api_list_objects": 11,
+  "device:api_list_objects": 20,
+  "device:list_objects_with_permission": 25,
+  "devicebay:api_list_objects": 14,
+  "devicebay:list_objects_with_permission": 21,
+  "devicebaytemplate:api_list_objects": 11,
+  "devicerole:api_list_objects": 13,
+  "devicerole:list_objects_with_permission": 20,
+  "devicetype:api_list_objects": 14,
+  "devicetype:list_objects_with_permission": 21,
+  "frontport:api_list_objects": 15,
+  "frontport:list_objects_with_permission": 25,
+  "frontporttemplate:api_list_objects": 12,
+  "interface:api_list_objects": 23,
+  "interface:list_objects_with_permission": 21,
+  "interfacetemplate:api_list_objects": 11,
+  "inventoryitem:api_list_objects": 20,
+  "inventoryitem:list_objects_with_permission": 23,
+  "inventoryitemrole:api_list_objects": 13,
+  "inventoryitemrole:list_objects_with_permission": 20,
+  "inventoryitemtemplate:api_list_objects": 13,
+  "location:api_list_objects": 15,
+  "location:list_objects_with_permission": 22,
+  "macaddress:api_list_objects": 17,
+  "macaddress:list_objects_with_permission": 24,
+  "manufacturer:api_list_objects": 13,
+  "manufacturer:list_objects_with_permission": 20,
+  "module:api_list_objects": 18,
+  "module:list_objects_with_permission": 24,
+  "modulebay:api_list_objects": 15,
+  "modulebay:list_objects_with_permission": 21,
+  "modulebaytemplate:api_list_objects": 11,
+  "moduletype:api_list_objects": 14,
+  "moduletype:list_objects_with_permission": 21,
+  "moduletypeprofile:api_list_objects": 13,
+  "moduletypeprofile:list_objects_with_permission": 20,
+  "platform:api_list_objects": 13,
+  "platform:list_objects_with_permission": 21,
+  "powerfeed:api_list_objects": 15,
+  "powerfeed:list_objects_with_permission": 22,
+  "poweroutlet:api_list_objects": 14,
+  "poweroutlet:list_objects_with_permission": 22,
+  "poweroutlettemplate:api_list_objects": 11,
+  "powerpanel:api_list_objects": 15,
+  "powerpanel:list_objects_with_permission": 22,
+  "powerport:api_list_objects": 14,
+  "powerport:list_objects_with_permission": 21,
+  "powerporttemplate:api_list_objects": 11,
+  "rack:api_list_objects": 17,
+  "rack:list_objects_with_permission": 29,
+  "rackgroup:api_list_objects": 13,
+  "rackgroup:list_objects_with_permission": 20,
+  "rackreservation:api_list_objects": 15,
+  "rackreservation:list_objects_with_permission": 23,
+  "rackrole:api_list_objects": 13,
+  "rackrole:list_objects_with_permission": 20,
+  "racktype:api_list_objects": 14,
+  "racktype:list_objects_with_permission": 21,
+  "rearport:api_list_objects": 15,
+  "rearport:list_objects_with_permission": 22,
+  "rearporttemplate:api_list_objects": 12,
+  "region:api_list_objects": 13,
+  "region:list_objects_with_permission": 20,
+  "site:api_list_objects": 16,
+  "site:list_objects_with_permission": 22,
+  "sitegroup:api_list_objects": 13,
+  "sitegroup:list_objects_with_permission": 20,
+  "virtualchassis:api_list_objects": 16,
+  "virtualchassis:list_objects_with_permission": 21,
+  "virtualdevicecontext:api_list_objects": 14,
+  "virtualdevicecontext:list_objects_with_permission": 20
+}

+ 37 - 0
netbox/extras/tests/query_counts.json

@@ -0,0 +1,37 @@
+{
+  "bookmark:api_list_objects": 12,
+  "bookmark:list_objects_with_permission": 17,
+  "configcontext:api_list_objects": 22,
+  "configcontext:list_objects_with_permission": 16,
+  "configcontextprofile:api_list_objects": 12,
+  "configcontextprofile:list_objects_with_permission": 19,
+  "configtemplate:api_list_objects": 10,
+  "configtemplate:list_objects_with_permission": 17,
+  "customfield:api_list_objects": 10,
+  "customfield:list_objects_with_permission": 19,
+  "customfieldchoiceset:api_list_objects": 9,
+  "customfieldchoiceset:list_objects_with_permission": 16,
+  "customlink:api_list_objects": 10,
+  "customlink:list_objects_with_permission": 18,
+  "eventrule:api_list_objects": 16,
+  "eventrule:list_objects_with_permission": 23,
+  "exporttemplate:api_list_objects": 10,
+  "exporttemplate:list_objects_with_permission": 19,
+  "imageattachment:api_list_objects": 11,
+  "imageattachment:list_objects_with_permission": 21,
+  "journalentry:api_list_objects": 15,
+  "journalentry:list_objects_with_permission": 24,
+  "notification:api_list_objects": 12,
+  "notificationgroup:api_list_objects": 11,
+  "notificationgroup:list_objects_with_permission": 18,
+  "savedfilter:api_list_objects": 10,
+  "savedfilter:list_objects_with_permission": 19,
+  "subscription:api_list_objects": 12,
+  "tableconfig:api_list_objects": 11,
+  "tableconfig:list_objects_with_permission": 19,
+  "tag:api_list_objects": 10,
+  "tag:list_objects_with_permission": 18,
+  "taggeditem:api_list_objects": 12,
+  "webhook:api_list_objects": 13,
+  "webhook:list_objects_with_permission": 20
+}

+ 37 - 0
netbox/ipam/tests/query_counts.json

@@ -0,0 +1,37 @@
+{
+  "aggregate:api_list_objects": 14,
+  "aggregate:list_objects_with_permission": 24,
+  "asn:api_list_objects": 18,
+  "asn:list_objects_with_permission": 31,
+  "asnrange:api_list_objects": 15,
+  "asnrange:list_objects_with_permission": 22,
+  "fhrpgroup:api_list_objects": 14,
+  "fhrpgroup:list_objects_with_permission": 21,
+  "fhrpgroupassignment:api_list_objects": 18,
+  "ipaddress:api_list_objects": 14,
+  "ipaddress:list_objects_with_permission": 21,
+  "iprange:api_list_objects": 13,
+  "iprange:list_objects_with_permission": 20,
+  "prefix:api_list_objects": 13,
+  "prefix:list_objects_with_permission": 29,
+  "rir:api_list_objects": 13,
+  "rir:list_objects_with_permission": 20,
+  "role:api_list_objects": 13,
+  "role:list_objects_with_permission": 20,
+  "routetarget:api_list_objects": 13,
+  "routetarget:list_objects_with_permission": 21,
+  "service:api_list_objects": 16,
+  "service:list_objects_with_permission": 21,
+  "servicetemplate:api_list_objects": 13,
+  "servicetemplate:list_objects_with_permission": 20,
+  "vlan:api_list_objects": 15,
+  "vlan:list_objects_with_permission": 24,
+  "vlangroup:api_list_objects": 13,
+  "vlangroup:list_objects_with_permission": 25,
+  "vlantranslationpolicy:api_list_objects": 12,
+  "vlantranslationpolicy:list_objects_with_permission": 20,
+  "vlantranslationrule:api_list_objects": 12,
+  "vlantranslationrule:list_objects_with_permission": 21,
+  "vrf:api_list_objects": 15,
+  "vrf:list_objects_with_permission": 20
+}

+ 14 - 0
netbox/tenancy/tests/query_counts.json

@@ -0,0 +1,14 @@
+{
+  "contact:api_list_objects": 14,
+  "contact:list_objects_with_permission": 21,
+  "contactassignment:api_list_objects": 17,
+  "contactassignment:list_objects_with_permission": 25,
+  "contactgroup:api_list_objects": 14,
+  "contactgroup:list_objects_with_permission": 20,
+  "contactrole:api_list_objects": 13,
+  "contactrole:list_objects_with_permission": 20,
+  "tenant:api_list_objects": 14,
+  "tenant:list_objects_with_permission": 21,
+  "tenantgroup:api_list_objects": 14,
+  "tenantgroup:list_objects_with_permission": 20
+}

+ 14 - 0
netbox/users/tests/query_counts.json

@@ -0,0 +1,14 @@
+{
+  "group:api_list_objects": 10,
+  "group:list_objects_with_permission": 17,
+  "objectpermission:api_list_objects": 14,
+  "objectpermission:list_objects_with_permission": 18,
+  "owner:api_list_objects": 11,
+  "owner:list_objects_with_permission": 19,
+  "ownergroup:api_list_objects": 9,
+  "ownergroup:list_objects_with_permission": 17,
+  "token:api_list_objects": 10,
+  "token:list_objects_with_permission": 17,
+  "user:api_list_objects": 12,
+  "user:list_objects_with_permission": 17
+}

+ 1 - 0
netbox/utilities/testing/__init__.py

@@ -1,6 +1,7 @@
 from .api import *
 from .base import *
 from .filtersets import *
+from .query_counts import *
 from .tables import *
 from .utils import *
 from .views import *

+ 3 - 1
netbox/utilities/testing/api.py

@@ -22,6 +22,7 @@ from users.models import ObjectPermission, Token, User
 from utilities.api import get_graphql_type_for_model
 
 from .base import ModelTestCase
+from .query_counts import assert_expected_query_count
 from .utils import disable_logging, disable_warnings, get_random_string
 
 __all__ = (
@@ -193,7 +194,8 @@ class APIViewTestCases:
             obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
 
             # Try GET to permitted objects
-            response = self.client.get(self._get_list_url(), **self.header)
+            with assert_expected_query_count(self, 'api_list_objects'):
+                response = self.client.get(self._get_list_url(), **self.header)
             self.assertHttpStatus(response, status.HTTP_200_OK)
             self.assertEqual(len(response.data['results']), 2)
 

+ 139 - 0
netbox/utilities/testing/query_counts.py

@@ -0,0 +1,139 @@
+import json
+import os
+import sys
+import threading
+from contextlib import contextmanager
+from pathlib import Path
+
+try:
+    import fcntl
+except ImportError:
+    fcntl = None
+
+from django.apps import apps as django_apps
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
+
+__all__ = (
+    'assert_expected_query_count',
+)
+
+UPDATE_ENV_VAR = 'UPDATE_QUERY_COUNTS'
+BASELINE_FILENAME = 'query_counts.json'
+
+_loaded_baselines = {}
+_lock = threading.Lock()
+
+
+def _is_update_mode():
+    return bool(os.environ.get(UPDATE_ENV_VAR))
+
+
+def _is_parallel_test_run():
+    # Heuristic: inspects sys.argv for Django's --parallel flag. This is
+    # sufficient for the project's standard `manage.py test` invocations but
+    # will not detect parallelism introduced by other test runners.
+    for arg in sys.argv:
+        if arg == '--parallel' or arg.startswith('--parallel='):
+            return True
+    return False
+
+
+def _baseline_path(app_label):
+    app_config = django_apps.get_app_config(app_label)
+    return Path(app_config.path) / 'tests' / BASELINE_FILENAME
+
+
+def _load_baseline(app_label):
+    with _lock:
+        if app_label in _loaded_baselines:
+            return _loaded_baselines[app_label]
+        path = _baseline_path(app_label)
+        if path.exists():
+            with path.open() as f:
+                data = json.load(f)
+        else:
+            data = {}
+        _loaded_baselines[app_label] = data
+        return data
+
+
+def _record_update(app_label, key, count):
+    # Write the baseline file synchronously rather than buffering until process
+    # exit, so updates are not lost if the runner terminates via os._exit() or
+    # a signal. An OS-level exclusive lock (where available) protects against
+    # concurrent processes — e.g. two simultaneous update-mode invocations —
+    # clobbering one another's writes.
+    with _lock:
+        path = _baseline_path(app_label)
+        path.parent.mkdir(parents=True, exist_ok=True)
+        with path.open('a+') as f:
+            if fcntl is not None:
+                fcntl.flock(f.fileno(), fcntl.LOCK_EX)
+            f.seek(0)
+            content = f.read()
+            existing = {}
+            if content:
+                try:
+                    existing = json.loads(content)
+                except json.JSONDecodeError:
+                    existing = {}
+            existing[key] = count
+            f.seek(0)
+            f.truncate()
+            json.dump(existing, f, indent=2, sort_keys=True)
+            f.write('\n')
+
+
+@contextmanager
+def assert_expected_query_count(test_case, name):
+    """
+    Assert that the wrapped block performs the number of SQL queries recorded
+    in the per-app baseline file (`<app>/tests/query_counts.json`).
+
+    The baseline key is `<model_name>:<name>`, derived from `test_case.model`.
+
+    When the `UPDATE_QUERY_COUNTS` environment variable is set, the assertion
+    is skipped and the observed count is written back to the baseline file
+    immediately. Update mode requires serial test execution (no --parallel).
+    """
+    model = test_case.model
+    app_label = model._meta.app_label
+    model_name = model._meta.model_name
+    key = f'{model_name}:{name}'
+
+    if _is_update_mode():
+        if _is_parallel_test_run():
+            raise RuntimeError(
+                f"{UPDATE_ENV_VAR}=1 cannot be combined with --parallel; "
+                f"re-run serially to regenerate query-count baselines."
+            )
+        ctx = CaptureQueriesContext(connection)
+        with ctx:
+            yield
+        _record_update(app_label, key, len(ctx.captured_queries))
+        return
+
+    baseline = _load_baseline(app_label)
+    if key not in baseline:
+        test_case.fail(
+            f"No query-count baseline recorded for {app_label}/{key}. "
+            f"Re-run with {UPDATE_ENV_VAR}=1 (serially) to record it."
+        )
+
+    expected = baseline[key]
+    ctx = CaptureQueriesContext(connection)
+    with ctx:
+        yield
+    actual = len(ctx.captured_queries)
+    if actual != expected:
+        sample = '\n'.join(
+            f"  {i + 1}. {q['sql'][:240]}"
+            for i, q in enumerate(ctx.captured_queries)
+        )
+        test_case.fail(
+            f"Query count for {app_label}/{key} changed: "
+            f"expected {expected}, got {actual}. "
+            f"If this change is intentional, re-run with {UPDATE_ENV_VAR}=1 "
+            f"to update the baseline.\nObserved queries:\n{sample}"
+        )

+ 4 - 1
netbox/utilities/testing/views.py

@@ -15,6 +15,7 @@ from netbox.models.features import ChangeLoggingMixin, CustomFieldsMixin
 from users.models import ObjectPermission
 
 from .base import ModelTestCase
+from .query_counts import assert_expected_query_count
 from .utils import add_custom_field_data, disable_warnings, get_random_string, post_data
 
 __all__ = (
@@ -470,7 +471,9 @@ class ViewTestCases:
             obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
 
             # Try GET with model-level permission
-            self.assertHttpStatus(self.client.get(self._get_url('list')), 200)
+            with assert_expected_query_count(self, 'list_objects_with_permission'):
+                response = self.client.get(self._get_url('list'))
+            self.assertHttpStatus(response, 200)
 
         def test_list_objects_with_constrained_permission(self):
             instance1, instance2 = self._get_queryset().all()[:2]

+ 16 - 0
netbox/virtualization/tests/query_counts.json

@@ -0,0 +1,16 @@
+{
+  "cluster:api_list_objects": 16,
+  "cluster:list_objects_with_permission": 22,
+  "clustergroup:api_list_objects": 13,
+  "clustergroup:list_objects_with_permission": 20,
+  "clustertype:api_list_objects": 13,
+  "clustertype:list_objects_with_permission": 20,
+  "virtualdisk:api_list_objects": 14,
+  "virtualdisk:list_objects_with_permission": 21,
+  "virtualmachine:api_list_objects": 18,
+  "virtualmachine:list_objects_with_permission": 24,
+  "virtualmachinetype:api_list_objects": 14,
+  "virtualmachinetype:list_objects_with_permission": 21,
+  "vminterface:api_list_objects": 19,
+  "vminterface:list_objects_with_permission": 21
+}

+ 22 - 0
netbox/vpn/tests/query_counts.json

@@ -0,0 +1,22 @@
+{
+  "ikepolicy:api_list_objects": 14,
+  "ikepolicy:list_objects_with_permission": 21,
+  "ikeproposal:api_list_objects": 13,
+  "ikeproposal:list_objects_with_permission": 20,
+  "ipsecpolicy:api_list_objects": 14,
+  "ipsecpolicy:list_objects_with_permission": 21,
+  "ipsecprofile:api_list_objects": 15,
+  "ipsecprofile:list_objects_with_permission": 22,
+  "ipsecproposal:api_list_objects": 13,
+  "ipsecproposal:list_objects_with_permission": 20,
+  "l2vpn:api_list_objects": 15,
+  "l2vpn:list_objects_with_permission": 20,
+  "l2vpntermination:api_list_objects": 16,
+  "l2vpntermination:list_objects_with_permission": 25,
+  "tunnel:api_list_objects": 14,
+  "tunnel:list_objects_with_permission": 21,
+  "tunnelgroup:api_list_objects": 13,
+  "tunnelgroup:list_objects_with_permission": 20,
+  "tunneltermination:api_list_objects": 18,
+  "tunneltermination:list_objects_with_permission": 28
+}

+ 8 - 0
netbox/wireless/tests/query_counts.json

@@ -0,0 +1,8 @@
+{
+  "wirelesslan:api_list_objects": 13,
+  "wirelesslan:list_objects_with_permission": 21,
+  "wirelesslangroup:api_list_objects": 13,
+  "wirelesslangroup:list_objects_with_permission": 21,
+  "wirelesslink:api_list_objects": 18,
+  "wirelesslink:list_objects_with_permission": 24
+}