2
0
Эх сурвалжийг харах

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

Jeremy Stretch 18 цаг өмнө
parent
commit
578dfd1cc7

+ 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)
 

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

@@ -0,0 +1,134 @@
+import atexit
+import json
+import os
+import sys
+import threading
+from contextlib import contextmanager
+from pathlib import Path
+
+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 = {}
+_pending_updates = {}
+_lock = threading.Lock()
+_atexit_registered = False
+
+
+def _is_update_mode():
+    return bool(os.environ.get(UPDATE_ENV_VAR))
+
+
+def _is_parallel_test_run():
+    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):
+    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):
+    global _atexit_registered
+    with _lock:
+        _pending_updates.setdefault(app_label, {})[key] = count
+        if not _atexit_registered:
+            atexit.register(_flush_updates)
+            _atexit_registered = True
+
+
+def _flush_updates():
+    with _lock:
+        for app_label, updates in _pending_updates.items():
+            path = _baseline_path(app_label)
+            path.parent.mkdir(parents=True, exist_ok=True)
+            existing = {}
+            if path.exists():
+                with path.open() as f:
+                    try:
+                        existing = json.load(f)
+                    except json.JSONDecodeError:
+                        existing = {}
+            existing.update(updates)
+            with path.open('w') as f:
+                json.dump(existing, f, indent=2, sort_keys=True)
+                f.write('\n')
+        _pending_updates.clear()
+
+
+@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 buffered for write-back at process
+    exit. 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
+}