From 492cb83cc3e9c30b7e28e16291e4f96dd6fd04da Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Mon, 3 Aug 2026 09:00:07 -0400 Subject: [PATCH 01/76] Fixes #22844: Allow null value for CustomFieldChoiceSet base_choices in REST API --- .../extras/api/serializers_/customfields.py | 3 +- netbox/extras/tests/test_api.py | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/netbox/extras/api/serializers_/customfields.py b/netbox/extras/api/serializers_/customfields.py index 175b88646..1cdbc46dd 100644 --- a/netbox/extras/api/serializers_/customfields.py +++ b/netbox/extras/api/serializers_/customfields.py @@ -19,7 +19,8 @@ __all__ = ( class CustomFieldChoiceSetSerializer(OwnerMixin, ChangeLogMessageSerializer, ValidatedModelSerializer): base_choices = ChoiceField( choices=CustomFieldChoiceSetBaseChoices, - required=False + required=False, + allow_null=True, ) extra_choices = serializers.ListField( child=serializers.ListField( diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index d278a5845..89af94be6 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -296,6 +296,36 @@ class CustomFieldChoiceSetTestCase(APIViewTestCases.APIViewTestCase): response = self.client.post(self._get_list_url(), data, format='json', **self.header) self.assertEqual(response.status_code, 400) + def test_null_base_choices(self): + """ + A null value for base_choices should be accepted, as returned by the API for a choice set which defines + only extra choices. + """ + self.add_permissions('extras.add_customfieldchoiceset', 'extras.change_customfieldchoiceset') + data = { + 'name': 'test', + 'base_choices': None, + 'extra_choices': [ + ['choice1', 'Choice 1'], + ], + } + + response = self.client.post(self._get_list_url(), data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + self.assertIsNone(response.data['base_choices']) + choice_set = CustomFieldChoiceSet.objects.get(pk=response.data['id']) + self.assertIsNone(choice_set.base_choices) + + # A choice set with base choices assigned can be reverted to null + choice_set.base_choices = CustomFieldChoiceSetBaseChoices.IATA + choice_set.save() + response = self.client.patch( + self._get_detail_url(choice_set), {'base_choices': None}, format='json', **self.header + ) + self.assertHttpStatus(response, status.HTTP_200_OK) + choice_set.refresh_from_db() + self.assertIsNone(choice_set.base_choices) + def test_invalid_choice_color(self): self.add_permissions('extras.add_customfieldchoiceset') data = { From a158ed3794c61d32e448cb23b0c0c854e087cc2c Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Mon, 3 Aug 2026 16:10:32 -0500 Subject: [PATCH 02/76] Fixes #22825: Handle CircuitTermination origins in cable path tracing CablePath.save() and delete() wrote the _path back-reference onto the path's origin object, and PathTraceView built the trace SVG URL from the origin's REST API action. Both assume the origin is a PathEndpoint, but a CircuitTermination is a valid cable-path origin (per from_origin) without the _path field or a -trace API action, so those paths raised FieldDoesNotExist and NoReverseMatch respectively. Guard the _path writes and the SVG URL on PathEndpoint membership, and skip the SVG block in the template when no URL is available. --- netbox/dcim/models/cables.py | 16 +++++++---- netbox/dcim/tests/test_cablepaths.py | 39 ++++++++++++++++++++++++++ netbox/dcim/views.py | 12 ++++++-- netbox/templates/dcim/cable_trace.html | 2 ++ 4 files changed, 60 insertions(+), 9 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 868ab0822..73b023913 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -780,18 +780,22 @@ class CablePath(models.Model): super().save(*args, **kwargs) - # Record a direct reference to this CablePath on its originating object(s) + # Record a direct reference to this CablePath on its originating object(s). Only PathEndpoint + # subclasses carry the denormalized `_path` back-reference; other valid origins (e.g. + # CircuitTermination) do not, so skip the update for them. origin_model = self.origin_type.model_class() - origin_ids = [decompile_path_node(node)[1] for node in self.path[0]] - origin_model.objects.filter(pk__in=origin_ids).update(_path=self.pk) + if issubclass(origin_model, PathEndpoint): + origin_ids = [decompile_path_node(node)[1] for node in self.path[0]] + origin_model.objects.filter(pk__in=origin_ids).update(_path=self.pk) def delete(self, *args, **kwargs): # Mirror save() - clear _path on origins to prevent stale references - # in table views that render _path.destinations + # in table views that render _path.destinations. Only PathEndpoint subclasses carry `_path`. if self.path: origin_model = self.origin_type.model_class() - origin_ids = [decompile_path_node(node)[1] for node in self.path[0]] - origin_model.objects.filter(pk__in=origin_ids, _path=self.pk).update(_path=None) + if issubclass(origin_model, PathEndpoint): + origin_ids = [decompile_path_node(node)[1] for node in self.path[0]] + origin_model.objects.filter(pk__in=origin_ids, _path=self.pk).update(_path=None) super().delete(*args, **kwargs) diff --git a/netbox/dcim/tests/test_cablepaths.py b/netbox/dcim/tests/test_cablepaths.py index d60f5cc86..ec7de6020 100644 --- a/netbox/dcim/tests/test_cablepaths.py +++ b/netbox/dcim/tests/test_cablepaths.py @@ -2680,6 +2680,45 @@ class LegacyCablePathTestCase(BaseCablePathTestCase): ) self.assertEqual(CablePath.objects.count(), 2) + def test_225_circuittermination_origin_passive_network(self): + """ + [CT1] --C1-- [RP1] [FP1] + + A CircuitTermination cabled into a passive (FrontPort/RearPort-only) device can become a + CablePath origin. Unlike PathEndpoint origins, CircuitTermination has no `_path` back-reference + field, so saving and deleting such a path must not attempt to write it (see #22825). + """ + rearport1 = RearPort.objects.create(device=self.device, name='Rear Port 1') + frontport1 = FrontPort.objects.create(device=self.device, name='Front Port 1') + PortMapping.objects.create( + device=self.device, front_port=frontport1, front_port_position=1, + rear_port=rearport1, rear_port_position=1, + ) + circuittermination1 = CircuitTermination.objects.create( + circuit=self.circuit, + termination=self.site, + term_side='A' + ) + cable1 = Cable( + a_terminations=[circuittermination1], + b_terminations=[rearport1] + ) + cable1.save() + + # Re-fetch so the in-memory instance reflects the cable set above (from_origin reads .cable). + circuittermination1.refresh_from_db() + + # A path traced from the CircuitTermination origin must save without raising FieldDoesNotExist + # on the missing `_path` field. + cablepath = CablePath.from_origin([circuittermination1]) + cablepath.save() + self.assertEqual(cablepath.origin_type.model_class(), CircuitTermination) + self.assertEqual(cablepath.origins, [circuittermination1]) + + # Deleting the path must likewise not attempt to clear a nonexistent `_path` field. + cablepath.delete() + self.assertIsNone(CablePath.objects.filter(pk=cablepath.pk).first()) + def test_301_create_path_via_existing_cable(self): """ [IF1] --C1-- [FP1] [RP1] --C2-- [RP2] [FP2] --C3-- [IF2] diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index cc888597d..987493641 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -208,9 +208,15 @@ class PathTraceView(generic.ObjectView): # Get the total length of the cable and whether the length is definitive (fully defined) total_length, is_definitive = path.get_total_length() if path else (None, False) - # Determine the path to the SVG trace image - api_viewname = f"{path.origin_type.app_label}-api:{path.origin_type.model}-trace" - svg_url = f"{reverse(api_viewname, kwargs={'pk': path.origins[0].pk})}?render=svg" + # Determine the path to the SVG trace image. The `-trace` API action (and the SVG renderer, + # which calls origin.trace()) exist only for PathEndpoint origins. Other valid origins such as + # CircuitTermination have no such action, so omit the SVG for them. + origin_model = path.origin_type.model_class() + if issubclass(origin_model, PathEndpoint): + api_viewname = f"{path.origin_type.app_label}-api:{path.origin_type.model}-trace" + svg_url = f"{reverse(api_viewname, kwargs={'pk': path.origins[0].pk})}?render=svg" + else: + svg_url = None return { 'path': path, diff --git a/netbox/templates/dcim/cable_trace.html b/netbox/templates/dcim/cable_trace.html index e07b12d81..bc7be377e 100644 --- a/netbox/templates/dcim/cable_trace.html +++ b/netbox/templates/dcim/cable_trace.html @@ -17,6 +17,7 @@ {# Cable trace SVG & options #}
{% if path %} + {% if svg_url %}
@@ -25,6 +26,7 @@
+ {% endif %}
{% if path.is_split and path.get_asymmetric_nodes %}

{% trans "Asymmetric Path" %}!

From c36e72876f026819e3b815cae0d4a24709234fe6 Mon Sep 17 00:00:00 2001 From: Jason Satein <8883967+clovehitch@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:36:11 -0700 Subject: [PATCH 03/76] Closes #22567: Warn that a custom script file name must not shadow an installed Python module (#22804) --- docs/customization/custom-scripts.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/customization/custom-scripts.md b/docs/customization/custom-scripts.md index f7aacb238..e18d7f700 100644 --- a/docs/customization/custom-scripts.md +++ b/docs/customization/custom-scripts.md @@ -23,6 +23,9 @@ Custom scripts are Python code which exists outside the NetBox code base, so the ## Writing Custom Scripts +!!! warning "Choose a unique file name" + A script file's name (without the `.py` extension) becomes its Python module name when the script is loaded. A script file must not share its name with a NetBox application (e.g. `circuits.py` or `dcim.py`) or any other installed Python module: the script will shadow that module in Python's import system and can break unrelated functionality. Choose a unique, descriptive file name, such as `circuit_maintenance.py`. + All custom scripts must inherit from the `extras.scripts.Script` base class. This class provides the functionality necessary to generate forms and log activity. ```python From 852f73b081a3a0ad5d63f76d377b235eaa81912f Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 4 Aug 2026 13:16:13 -0400 Subject: [PATCH 04/76] Release v4.6.8-rc2 --- netbox/release.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/netbox/release.yaml b/netbox/release.yaml index ecc1b766a..b9f189b9e 100644 --- a/netbox/release.yaml +++ b/netbox/release.yaml @@ -1,3 +1,4 @@ -version: "4.6.7" +version: "4.6.8" edition: "Community" -published: "2026-07-30" +build: "rc2" +published: "2026-08-04" From 3d3bebcb785ba89e188f313c4b224dd319714460 Mon Sep 17 00:00:00 2001 From: Elliott Balsley <3991046+llamafilm@users.noreply.github.com> Date: Tue, 4 Aug 2026 21:28:20 +0200 Subject: [PATCH 05/76] Closes #22787: Improve GraphQL query efficiency when resolving assigned objects (#22792) --- netbox/circuits/graphql/types.py | 27 ++++- netbox/dcim/graphql/types.py | 101 +++++++++++++---- netbox/ipam/graphql/types.py | 72 +++++++++++- netbox/netbox/graphql/optimization.py | 43 +++++++ netbox/netbox/tests/test_graphql.py | 150 +++++++++++++++++++++++++ netbox/virtualization/graphql/types.py | 15 ++- netbox/vpn/graphql/types.py | 16 ++- netbox/wireless/graphql/types.py | 15 ++- 8 files changed, 408 insertions(+), 31 deletions(-) create mode 100644 netbox/netbox/graphql/optimization.py diff --git a/netbox/circuits/graphql/types.py b/netbox/circuits/graphql/types.py index d150d2b5c..65be61d7f 100644 --- a/netbox/circuits/graphql/types.py +++ b/netbox/circuits/graphql/types.py @@ -5,7 +5,9 @@ import strawberry_django from circuits import models from dcim.graphql.mixins import CabledObjectMixin +from dcim.models import Location, Region, Site, SiteGroup from extras.graphql.mixins import ContactsMixin, CustomFieldsMixin, TagsMixin +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.types import BaseObjectType, ObjectType, OrganizationalObjectType, PrimaryObjectType from tenancy.graphql.types import TenantType @@ -74,7 +76,19 @@ class ProviderNetworkType(PrimaryObjectType): class CircuitTerminationType(CustomFieldsMixin, TagsMixin, CabledObjectMixin, ObjectType): circuit: Annotated['CircuitType', strawberry.lazy('circuits.graphql.types')] - @strawberry_django.field(prefetch_related='termination') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'termination', + [ + Location, + Region, + SiteGroup, + Site, + models.ProviderNetwork, + ], + ), + only=['termination_type', 'termination_id'], + ) def termination(self) -> Annotated[ Annotated['LocationType', strawberry.lazy('dcim.graphql.types')] | Annotated['RegionType', strawberry.lazy('dcim.graphql.types')] @@ -133,7 +147,16 @@ class CircuitGroupType(OrganizationalObjectType): class CircuitGroupAssignmentType(TagsMixin, BaseObjectType): group: Annotated['CircuitGroupType', strawberry.lazy('circuits.graphql.types')] - @strawberry_django.field(prefetch_related='member') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'member', + [ + models.Circuit, + models.VirtualCircuit, + ], + ), + only=['member_type', 'member_id'], + ) def member(self) -> Annotated[ Annotated['CircuitType', strawberry.lazy('circuits.graphql.types')] | Annotated['VirtualCircuitType', strawberry.lazy('circuits.graphql.types')], diff --git a/netbox/dcim/graphql/types.py b/netbox/dcim/graphql/types.py index 7f3d01bcc..e4f4b7a14 100644 --- a/netbox/dcim/graphql/types.py +++ b/netbox/dcim/graphql/types.py @@ -9,6 +9,7 @@ from core.graphql.mixins import ChangelogMixin from dcim import models from extras.graphql.mixins import ConfigContextMixin, ContactsMixin, ImageAttachmentsMixin from ipam.graphql.mixins import IPAddressesMixin, VLANGroupsMixin +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.scalars import BigInt from netbox.graphql.types import ( BaseObjectType, @@ -19,7 +20,7 @@ from netbox.graphql.types import ( ) from users.graphql.mixins import OwnerMixin from utilities.querysets import RestrictedPrefetch -from virtualization.models import Cluster +from virtualization.models import Cluster, VMInterface from .filters import * from .mixins import CabledObjectMixin, PathEndpointMixin @@ -150,7 +151,25 @@ class CableBundleType(PrimaryObjectType): ) class CableTerminationType(NetBoxObjectType): cable: Annotated['CableType', strawberry.lazy('dcim.graphql.types')] | None - termination: Annotated[ + + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'termination', + [ + CircuitTermination, + models.ConsolePort, + models.ConsoleServerPort, + models.FrontPort, + models.Interface, + models.PowerFeed, + models.PowerOutlet, + models.PowerPort, + models.RearPort, + ], + ), + only=['termination_type', 'termination_id'], + ) + def termination(self) -> Annotated[ Annotated['CircuitTerminationType', strawberry.lazy('circuits.graphql.types')] | Annotated['ConsolePortType', strawberry.lazy('dcim.graphql.types')] | Annotated['ConsoleServerPortType', strawberry.lazy('dcim.graphql.types')] @@ -161,7 +180,8 @@ class CableTerminationType(NetBoxObjectType): | Annotated['PowerPortType', strawberry.lazy('dcim.graphql.types')] | Annotated['RearPortType', strawberry.lazy('dcim.graphql.types')], strawberry.union('CableTerminationTerminationType'), - ] | None + ] | None: + return self.termination @strawberry_django.type( @@ -330,22 +350,38 @@ class InventoryItemTemplateType(ComponentTemplateType): role: Annotated['InventoryItemRoleType', strawberry.lazy('dcim.graphql.types')] | None manufacturer: Annotated['ManufacturerType', strawberry.lazy('dcim.graphql.types')] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field(prefetch_related='parent', only=['parent_id']) def parent(self) -> Annotated['InventoryItemTemplateType', strawberry.lazy('dcim.graphql.types')] | None: return self.parent child_items: list[Annotated['InventoryItemTemplateType', strawberry.lazy('dcim.graphql.types')]] - component: Annotated[ - Annotated['ConsolePortType', strawberry.lazy('dcim.graphql.types')] - | Annotated['ConsoleServerPortType', strawberry.lazy('dcim.graphql.types')] - | Annotated['FrontPortType', strawberry.lazy('dcim.graphql.types')] - | Annotated['InterfaceType', strawberry.lazy('dcim.graphql.types')] - | Annotated['PowerOutletType', strawberry.lazy('dcim.graphql.types')] - | Annotated['PowerPortType', strawberry.lazy('dcim.graphql.types')] - | Annotated['RearPortType', strawberry.lazy('dcim.graphql.types')], + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'component', + [ + models.ConsolePortTemplate, + models.ConsoleServerPortTemplate, + models.FrontPortTemplate, + models.InterfaceTemplate, + models.PowerOutletTemplate, + models.PowerPortTemplate, + models.RearPortTemplate, + ], + ), + only=['component_type', 'component_id'], + ) + def component(self) -> Annotated[ + Annotated['ConsolePortTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['ConsoleServerPortTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['FrontPortTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['InterfaceTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['PowerOutletTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['PowerPortTemplateType', strawberry.lazy('dcim.graphql.types')] + | Annotated['RearPortTemplateType', strawberry.lazy('dcim.graphql.types')], strawberry.union('InventoryItemTemplateComponentType'), - ] | None + ] | None: + return self.component @strawberry_django.type( @@ -433,7 +469,16 @@ class FrontPortTemplateType(ModularComponentTemplateType): class MACAddressType(PrimaryObjectType): mac_address: str - @strawberry_django.field(prefetch_related='assigned_object') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'assigned_object', + [ + models.Interface, + VMInterface, + ], + ), + only=['assigned_object_type', 'assigned_object_id'], + ) def assigned_object(self) -> Annotated[ Annotated['InterfaceType', strawberry.lazy('dcim.graphql.types')] | Annotated['VMInterfaceType', strawberry.lazy('virtualization.graphql.types')], @@ -497,11 +542,26 @@ class InventoryItemType(ComponentType): child_items: list[Annotated['InventoryItemType', strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field(prefetch_related='parent', only=['parent_id']) def parent(self) -> Annotated['InventoryItemType', strawberry.lazy('dcim.graphql.types')] | None: return self.parent - component: Annotated[ + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'component', + [ + models.ConsolePort, + models.ConsoleServerPort, + models.FrontPort, + models.Interface, + models.PowerOutlet, + models.PowerPort, + models.RearPort, + ], + ), + only=['component_type', 'component_id'], + ) + def component(self) -> Annotated[ Annotated['ConsolePortType', strawberry.lazy('dcim.graphql.types')] | Annotated['ConsoleServerPortType', strawberry.lazy('dcim.graphql.types')] | Annotated['FrontPortType', strawberry.lazy('dcim.graphql.types')] @@ -510,7 +570,8 @@ class InventoryItemType(ComponentType): | Annotated['PowerPortType', strawberry.lazy('dcim.graphql.types')] | Annotated['RearPortType', strawberry.lazy('dcim.graphql.types')], strawberry.union('InventoryItemComponentType'), - ] | None + ] | None: + return self.component @strawberry_django.type( @@ -611,7 +672,7 @@ class ModuleBayType(ModularComponentType): installed_module: Annotated["ModuleType", strawberry.lazy('dcim.graphql.types')] | None children: list[Annotated["ModuleBayType", strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field(prefetch_related='parent', only=['parent_id']) def parent(self) -> Annotated["ModuleBayType", strawberry.lazy('dcim.graphql.types')] | None: return self.parent @@ -888,7 +949,7 @@ class RegionType(VLANGroupsMixin, ContactsMixin, NestedGroupObjectType): sites: list[Annotated["SiteType", strawberry.lazy('dcim.graphql.types')]] children: list[Annotated["RegionType", strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field(prefetch_related='parent', only=['parent_id']) def parent(self) -> Annotated["RegionType", strawberry.lazy('dcim.graphql.types')] | None: return self.parent @@ -965,7 +1026,7 @@ class SiteGroupType(VLANGroupsMixin, ContactsMixin, NestedGroupObjectType): sites: list[Annotated["SiteType", strawberry.lazy('dcim.graphql.types')]] children: list[Annotated["SiteGroupType", strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field(prefetch_related='parent', only=['parent_id']) def parent(self) -> Annotated["SiteGroupType", strawberry.lazy('dcim.graphql.types')] | None: return self.parent diff --git a/netbox/ipam/graphql/types.py b/netbox/ipam/graphql/types.py index b684022da..667f2142f 100644 --- a/netbox/ipam/graphql/types.py +++ b/netbox/ipam/graphql/types.py @@ -5,10 +5,13 @@ import strawberry_django from circuits.graphql.types import ProviderType from dcim.graphql.types import SiteType +from dcim.models import Device, Interface, Location, Rack, RackGroup, Region, Site, SiteGroup from extras.graphql.mixins import ContactsMixin from ipam import models +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.scalars import BigInt from netbox.graphql.types import BaseObjectType, NetBoxObjectType, OrganizationalObjectType, PrimaryObjectType +from virtualization.models import Cluster, ClusterGroup, VirtualMachine, VMInterface from .filters import * from .mixins import IPAddressesMixin @@ -18,6 +21,7 @@ if TYPE_CHECKING: DeviceType, InterfaceType, LocationType, + RackGroupType, RackType, RegionType, SiteGroupType, @@ -126,7 +130,16 @@ class FHRPGroupType(IPAddressesMixin, PrimaryObjectType): class FHRPGroupAssignmentType(BaseObjectType): group: Annotated['FHRPGroupType', strawberry.lazy('ipam.graphql.types')] - @strawberry_django.field(prefetch_related='interface') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'interface', + [ + Interface, + VMInterface, + ], + ), + only=['interface_type', 'interface_id'], + ) def interface(self) -> Annotated[ Annotated['InterfaceType', strawberry.lazy('dcim.graphql.types')] | Annotated['VMInterfaceType', strawberry.lazy('virtualization.graphql.types')], @@ -155,7 +168,17 @@ class IPAddressType(ContactsMixin, PrimaryObjectType): def family(self) -> IPAddressFamilyType: return IPAddressFamilyType(value=self.family, label=f'IPv{self.family}') - @strawberry_django.field(prefetch_related='assigned_object') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'assigned_object', + [ + models.FHRPGroup, + Interface, + VMInterface, + ], + ), + only=['assigned_object_type', 'assigned_object_id'], + ) def assigned_object(self) -> Annotated[ Annotated['InterfaceType', strawberry.lazy('dcim.graphql.types')] | Annotated['FHRPGroupType', strawberry.lazy('ipam.graphql.types')] @@ -197,7 +220,18 @@ class PrefixType(ContactsMixin, PrimaryObjectType): def family(self) -> IPAddressFamilyType: return IPAddressFamilyType(value=self.family, label=f'IPv{self.family}') - @strawberry_django.field(prefetch_related='scope') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'scope', + [ + Region, + SiteGroup, + Site, + Location, + ], + ), + only=['scope_type', 'scope_id'], + ) def scope(self) -> Annotated[ Annotated['LocationType', strawberry.lazy('dcim.graphql.types')] | Annotated['RegionType', strawberry.lazy('dcim.graphql.types')] @@ -259,7 +293,17 @@ class ServiceType(ContactsMixin, PrimaryObjectType): ports: list[int] ipaddresses: list[Annotated['IPAddressType', strawberry.lazy('ipam.graphql.types')]] - @strawberry_django.field(prefetch_related='parent') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'parent', + [ + Device, + VirtualMachine, + models.FHRPGroup, + ], + ), + only=['parent_object_type', 'parent_object_id'], + ) def parent(self) -> Annotated[ Annotated['DeviceType', strawberry.lazy('dcim.graphql.types')] | Annotated['VirtualMachineType', strawberry.lazy('virtualization.graphql.types')] @@ -298,7 +342,7 @@ class VLANType(PrimaryObjectType): interfaces_as_tagged: list[Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')]] vminterfaces_as_tagged: list[Annotated["VMInterfaceType", strawberry.lazy('virtualization.graphql.types')]] - @strawberry_django.field(prefetch_related='qinq_svlan') + @strawberry_django.field(prefetch_related='qinq_svlan', only=['qinq_svlan_id']) def qinq_svlan(self) -> Annotated["VLANType", strawberry.lazy('ipam.graphql.types')] | None: return self.qinq_svlan @@ -316,11 +360,27 @@ class VLANGroupType(OrganizationalObjectType): total_vlan_ids: BigInt tenant: Annotated['TenantType', strawberry.lazy('tenancy.graphql.types')] | None - @strawberry_django.field(prefetch_related='scope') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'scope', + [ + Cluster, + ClusterGroup, + Location, + Rack, + RackGroup, + Region, + Site, + SiteGroup, + ], + ), + only=['scope_type', 'scope_id'], + ) def scope(self) -> Annotated[ Annotated['ClusterType', strawberry.lazy('virtualization.graphql.types')] | Annotated['ClusterGroupType', strawberry.lazy('virtualization.graphql.types')] | Annotated['LocationType', strawberry.lazy('dcim.graphql.types')] + | Annotated['RackGroupType', strawberry.lazy('dcim.graphql.types')] | Annotated['RackType', strawberry.lazy('dcim.graphql.types')] | Annotated['RegionType', strawberry.lazy('dcim.graphql.types')] | Annotated['SiteType', strawberry.lazy('dcim.graphql.types')] diff --git a/netbox/netbox/graphql/optimization.py b/netbox/netbox/graphql/optimization.py new file mode 100644 index 000000000..d5263fae9 --- /dev/null +++ b/netbox/netbox/graphql/optimization.py @@ -0,0 +1,43 @@ +from collections.abc import Callable, Sequence + +from django.contrib.contenttypes.prefetch import GenericPrefetch +from django.db.models import Model, QuerySet +from strawberry.types import Info +from strawberry_django.optimizer import optimize +from strawberry_django.optimizer import optimizer as optimizer_ctx + +__all__ = ( + 'build_gfk_prefetch', + 'optimize_prefetch_queryset', +) + + +def optimize_prefetch_queryset(queryset: QuerySet, info: Info) -> QuerySet: + """ + Apply strawberry-django's query optimizer to a queryset used inside a GenericForeignKey prefetch. + """ + if ext := optimizer_ctx.get(): + return ext.optimize(queryset, info) + + return optimize(queryset, info) + + +def build_gfk_prefetch( + lookup: str, + models: Sequence[type[Model]], +) -> Callable[[Info], GenericPrefetch]: + """ + Return a selection-aware GenericPrefetch for a GenericForeignKey field. + + Each model gets its own queryset, optimized according to the client's GraphQL selection set. + """ + + def prefetch(info: Info) -> GenericPrefetch: + querysets = [ + optimize_prefetch_queryset(model.objects.all(), info) + for model in models + ] + + return GenericPrefetch(lookup, querysets) + + return prefetch diff --git a/netbox/netbox/tests/test_graphql.py b/netbox/netbox/tests/test_graphql.py index 46f06dd6e..098e7d0e0 100644 --- a/netbox/netbox/tests/test_graphql.py +++ b/netbox/netbox/tests/test_graphql.py @@ -18,6 +18,7 @@ from dcim.models import ( Device, DeviceRole, DeviceType, + Interface, Location, Manufacturer, Rack, @@ -35,6 +36,12 @@ from utilities.tables import get_table_for_model from utilities.testing import APITestCase, APIViewTestCases, TestCase, disable_warnings +def count_primary_table_queries(queries, table): + """Count queries that read from `table` as the primary relation (not only as a join).""" + pattern = re.compile(rf'FROM "{re.escape(table)}"') + return sum(1 for query_record in queries if pattern.search(query_record['sql'])) + + class GraphQLTestCase(TestCase): def _schema_extension_instances(self): @@ -469,6 +476,149 @@ class GraphQLAPITestCase(APITestCase): msg=f'Expected batched tag prefetch, got {tag_queries} tag queries for 10 devices', ) + def test_graphql_ip_address_list_assigned_object(self): + """ + Requesting assigned_object should batch prefetch related objects. + """ + self.add_permissions('ipam.view_ipaddress', 'dcim.view_interface', 'dcim.view_device') + + site = Site.objects.first() + manufacturer = Manufacturer.objects.create(name='Assigned Object Manufacturer', slug='assigned-object-mfg') + device_type = DeviceType.objects.create( + manufacturer=manufacturer, + model='Assigned Object Model', + slug='assigned-object-model', + ) + device_role = DeviceRole.objects.create(name='Assigned Object Role', slug='assigned-object-role') + device = Device.objects.create( + name='Assigned Object Device', + site=site, + device_type=device_type, + role=device_role, + ) + interface = Interface.objects.create(name='eth0', device=device, type='1000baset') + ip_addresses = IPAddress.objects.bulk_create([ + IPAddress(address=f'192.0.2.{index}/24', assigned_object=interface) + for index in range(1, 6) + ]) + ip_ids = json.dumps([str(ip.pk) for ip in ip_addresses]) + + query = f""" + {{ + ip_address_list(filters: {{id: {{in_list: {ip_ids}}}}}) {{ + address + assigned_object {{ + ... on InterfaceType {{ + name + device {{ + name + }} + }} + }} + }} + }} + """ + 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) + self.assertEqual(len(data['data']['ip_address_list']), len(ip_addresses)) + + device_queries = count_primary_table_queries(context.captured_queries, 'dcim_device') + self.assertLessEqual( + device_queries, + 2, + msg=f'Expected batched assigned_object prefetch, got {device_queries} device queries for 5 IP addresses', + ) + + def test_graphql_ip_address_list_assigned_object_nested_site(self): + """ + Nested assigned_object selections should be optimized on the GFK prefetch queryset. + """ + self.add_permissions( + 'ipam.view_ipaddress', + 'dcim.view_interface', + 'dcim.view_device', + 'dcim.view_site', + ) + + site = Site.objects.first() + manufacturer = Manufacturer.objects.create( + name='Nested Site Manufacturer', + slug='nested-site-mfg', + ) + device_type = DeviceType.objects.create( + manufacturer=manufacturer, + model='Nested Site Model', + slug='nested-site-model', + ) + device_role = DeviceRole.objects.create(name='Nested Site Role', slug='nested-site-role') + interfaces = [] + for index in range(5): + device = Device.objects.create( + name=f'Nested Site Device {index}', + site=site, + device_type=device_type, + role=device_role, + ) + interfaces.append(Interface.objects.create( + name=f'eth{index}', + device=device, + type='1000baset', + )) + ip_addresses = IPAddress.objects.bulk_create([ + IPAddress(address=f'192.0.2.{index}/24', assigned_object=interfaces[index - 1]) + for index in range(1, 6) + ]) + ip_ids = json.dumps([str(ip.pk) for ip in ip_addresses]) + + query = f""" + {{ + ip_address_list(filters: {{id: {{in_list: {ip_ids}}}}}) {{ + address + assigned_object {{ + ... on InterfaceType {{ + name + device {{ + name + site {{ + name + }} + }} + }} + }} + }} + }} + """ + 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) + self.assertEqual(len(data['data']['ip_address_list']), len(ip_addresses)) + for ip_data in data['data']['ip_address_list']: + self.assertEqual(ip_data['assigned_object']['device']['site']['name'], site.name) + + device_queries = count_primary_table_queries(context.captured_queries, 'dcim_device') + site_queries = count_primary_table_queries(context.captured_queries, 'dcim_site') + self.assertLessEqual( + device_queries, + 2, + msg=f'Expected batched device prefetch, got {device_queries} device queries for 5 IP addresses', + ) + self.assertLessEqual( + site_queries, + 2, + msg=f'Expected optimized site join, got {site_queries} site queries for 5 IP addresses', + ) + def test_offset_pagination(self): self.add_permissions('dcim.view_site') url = reverse('graphql') diff --git a/netbox/virtualization/graphql/types.py b/netbox/virtualization/graphql/types.py index 81ef685a1..dbad15d54 100644 --- a/netbox/virtualization/graphql/types.py +++ b/netbox/virtualization/graphql/types.py @@ -3,8 +3,10 @@ from typing import TYPE_CHECKING, Annotated import strawberry import strawberry_django +from dcim.models import Location, Region, Site, SiteGroup from extras.graphql.mixins import ConfigContextMixin, ContactsMixin from ipam.graphql.mixins import IPAddressesMixin, VLANGroupsMixin +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.scalars import BigInt from netbox.graphql.types import NetBoxObjectType, OrganizationalObjectType, PrimaryObjectType from users.graphql.mixins import OwnerMixin @@ -59,7 +61,18 @@ class ClusterType(ContactsMixin, VLANGroupsMixin, PrimaryObjectType): virtual_machines: list[Annotated["VirtualMachineType", strawberry.lazy('virtualization.graphql.types')]] devices: list[Annotated["DeviceType", strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='scope') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'scope', + [ + Region, + SiteGroup, + Site, + Location, + ], + ), + only=['scope_type', 'scope_id'], + ) def scope(self) -> Annotated[ Annotated['LocationType', strawberry.lazy('dcim.graphql.types')] | Annotated['RegionType', strawberry.lazy('dcim.graphql.types')] diff --git a/netbox/vpn/graphql/types.py b/netbox/vpn/graphql/types.py index 40f125e83..1d7216599 100644 --- a/netbox/vpn/graphql/types.py +++ b/netbox/vpn/graphql/types.py @@ -3,8 +3,12 @@ from typing import TYPE_CHECKING, Annotated import strawberry import strawberry_django +from dcim.models import Interface from extras.graphql.mixins import ContactsMixin, CustomFieldsMixin, TagsMixin +from ipam.models import VLAN +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.types import NetBoxObjectType, ObjectType, OrganizationalObjectType, PrimaryObjectType +from virtualization.models import VMInterface from vpn import models from .filters import * @@ -145,7 +149,17 @@ class L2VPNType(ContactsMixin, PrimaryObjectType): class L2VPNTerminationType(NetBoxObjectType): l2vpn: Annotated["L2VPNType", strawberry.lazy('vpn.graphql.types')] - @strawberry_django.field(prefetch_related='assigned_object') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'assigned_object', + [ + Interface, + VMInterface, + VLAN, + ], + ), + only=['assigned_object_type', 'assigned_object_id'], + ) def assigned_object(self) -> Annotated[ Annotated['InterfaceType', strawberry.lazy('dcim.graphql.types')] | Annotated['VLANType', strawberry.lazy('ipam.graphql.types')] diff --git a/netbox/wireless/graphql/types.py b/netbox/wireless/graphql/types.py index 557f32ce4..589fc237c 100644 --- a/netbox/wireless/graphql/types.py +++ b/netbox/wireless/graphql/types.py @@ -3,6 +3,8 @@ from typing import TYPE_CHECKING, Annotated import strawberry import strawberry_django +from dcim.models import Location, Region, Site, SiteGroup +from netbox.graphql.optimization import build_gfk_prefetch from netbox.graphql.types import NestedGroupObjectType, PrimaryObjectType from wireless import models @@ -46,7 +48,18 @@ class WirelessLANType(PrimaryObjectType): interfaces: list[Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')]] - @strawberry_django.field(prefetch_related='scope') + @strawberry_django.field( + prefetch_related=build_gfk_prefetch( + 'scope', + [ + Region, + SiteGroup, + Site, + Location, + ], + ), + only=['scope_type', 'scope_id'], + ) def scope(self) -> Annotated[ Annotated['LocationType', strawberry.lazy('dcim.graphql.types')] | Annotated['RegionType', strawberry.lazy('dcim.graphql.types')] From 0701a42a9434c47346e7f2300356c141d2f931cb Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 4 Aug 2026 16:39:35 -0500 Subject: [PATCH 06/76] Fixes #22812: Avoid loading all jobs into memory when deleting a JobsMixin object Deleting a Script (or any JobsMixin object) with thousands of associated jobs could consume several GB of memory and exhaust the host, because Django's deletion collector loads every related Job into memory. Jobs can never be fast-deleted (a global pre_delete receiver forces per-instance signal dispatch), and each Job carries potentially large data and log_entries payloads. Two paths loaded the full job set independently, so both are addressed: - The delete cascade: JobsMixin.delete() now deletes the object's jobs in batches before delegating to super().delete(), wrapped in a transaction so a failure in the parent delete rolls the job deletions back. After the loop the cascade collector finds no jobs to materialize. - The delete-confirmation page: _get_dependent_objects() uses a ConfirmCollector that counts the jobs relation rather than descending into it, so the page never instantiates the jobs. Counted relations render as a non-expandable row (via a CountOnly stand-in) alongside the itemized dependents. --- netbox/extras/tests/test_scripts_deletion.py | 236 +++++++++++++++++++ netbox/netbox/constants.py | 5 + netbox/netbox/models/deletion.py | 56 +++++ netbox/netbox/models/features.py | 22 +- netbox/netbox/views/generic/object_views.py | 21 +- netbox/templates/htmx/delete_form.html | 53 +++-- 6 files changed, 363 insertions(+), 30 deletions(-) create mode 100644 netbox/extras/tests/test_scripts_deletion.py diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py new file mode 100644 index 000000000..9aaf11e0c --- /dev/null +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -0,0 +1,236 @@ +import uuid +from unittest import mock + +from django.contrib.contenttypes.models import ContentType +from django.db import router +from django.db.models import QuerySet +from django.test import TestCase, override_settings +from django.urls import reverse + +from core.choices import ManagedFileRootPathChoices +from core.models import DataSource, Job +from extras.models import Script, ScriptModule +from extras.validators import CustomValidator +from netbox.models.deletion import ConfirmCollector, CountOnly +from utilities.exceptions import AbortRequest +from utilities.testing import TestCase as ViewTestCase + + +class ScriptDeletionTestCase(TestCase): + """ + Regression tests for #22812: deleting a JobsMixin object (Script, ScriptModule, DataSource) + with many associated Jobs must not load every Job into memory at once. + """ + @classmethod + def setUpTestData(cls): + cls.script_ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + + def _create_module(self): + return ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + + def _create_script(self, module=None): + module = module or self._create_module() + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + return module, script + + def _add_jobs(self, obj, count, object_type=None): + object_type = object_type or ContentType.objects.get_for_model(type(obj), for_concrete_model=False) + Job.objects.bulk_create([ + Job( + object_type=object_type, + object_id=obj.pk, + name='testjob', + status='completed', + job_id=uuid.uuid4(), + data={'output': 'x' * 50}, + ) + for _ in range(count) + ]) + + def test_delete_script_deletes_all_jobs(self): + _, script = self._create_script() + self._add_jobs(script, 2500) + self.assertEqual(script.jobs.count(), 2500) + + script.delete() + + self.assertFalse(Script.objects.filter(pk=script.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + + def test_delete_script_batches_jobs(self): + _, script = self._create_script() + self._add_jobs(script, 5) + + job_delete_calls = [] + original_delete = QuerySet.delete + + def counting_delete(qs, *args, **kwargs): + if qs.model is Job: + job_delete_calls.append(len(qs)) + return original_delete(qs, *args, **kwargs) + + with mock.patch('netbox.models.features.JOB_DELETE_BATCH_SIZE', 2): + with mock.patch.object(QuerySet, 'delete', counting_delete): + script.delete() + + # 5 jobs at a batch size of 2 => three batched deletes (2, 2, 1) + self.assertEqual(job_delete_calls, [2, 2, 1]) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + + def test_delete_scriptmodule_cascades_to_scripts_and_jobs(self): + module, script = self._create_script() + self._add_jobs(script, 100) + + module.delete() + + self.assertFalse(ScriptModule.objects.filter(pk=module.pk).exists()) + self.assertFalse(Script.objects.filter(pk=script.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + + def test_delete_datasource_deletes_jobs(self): + datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test') + self._add_jobs(datasource, 100) + ds_ct = ContentType.objects.get_for_model(DataSource, for_concrete_model=False) + self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 100) + + datasource.delete() + + self.assertFalse(DataSource.objects.filter(pk=datasource.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 0) + + def test_soft_delete_preserves_jobs(self): + _, script = self._create_script() + self._add_jobs(script, 10) + + script.delete(soft_delete=True) + + script.refresh_from_db() + self.assertFalse(script.is_executable) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10) + + @override_settings(PROTECTION_RULES={'extras.script': [CustomValidator({'name': {'eq': ''}})]}) + def test_delete_rolls_back_jobs_on_parent_failure(self): + # A protection rule that no real script can satisfy (name must be empty) makes the + # cascade's pre_delete handler raise AbortRequest *after* JobsMixin.delete has already + # batch-deleted the jobs. JobsMixin.delete wraps the batch loop and super().delete() in a + # transaction, so the job deletions must roll back, leaving no orphaned partial state. + # This exercises the real deletion-abort path rather than mocking Django internals. + _, script = self._create_script() + self._add_jobs(script, 10) + + with self.assertRaises(AbortRequest): + script.delete() + + self.assertTrue(Script.objects.filter(pk=script.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10) + + +class ConfirmCollectorTestCase(TestCase): + """ + #22812: the delete-confirmation page must not materialize every dependent Job. + """ + def _create_script_with_jobs(self, count): + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', + job_id=uuid.uuid4(), data={'output': 'x' * 50}) + for _ in range(count) + ]) + return script + + def test_confirm_collector_counts_jobs_without_instantiating(self): + script = self._create_script_with_jobs(500) + + init_calls = [] + original_init = Job.__init__ + + def counting_init(self, *args, **kwargs): + init_calls.append(1) + original_init(self, *args, **kwargs) + + with mock.patch.object(Job, '__init__', counting_init): + collector = ConfirmCollector(using=router.db_for_write(Script)) + collector.collect([script]) + + # No Job rows were instantiated; the relation was counted instead. + self.assertEqual(len(init_calls), 0) + self.assertNotIn(Job, collector.data) + self.assertEqual(collector.generic_relation_counts.get(Job), 500) + # The non-job cascade (the Script itself) is still collected. + self.assertIn(Script, collector.data) + + def test_count_only_wrapper(self): + # CountOnly reports its count via len() but iterates empty, so it slots into the + # dependent-objects mapping as a non-expandable, non-materializing row. + wrapper = CountOnly(3000) + self.assertEqual(len(wrapper), 3000) + self.assertEqual(list(wrapper), []) + self.assertTrue(wrapper.count_only) + + +class ObjectDeleteViewCountsTestCase(ViewTestCase): + """ + #22812: the delete-confirmation view must report a JobsMixin object's jobs as a count + (via CountOnly) without materializing them, and _get_dependent_objects must keep returning + a single dict. + """ + def test_get_dependent_objects_returns_count_only_for_jobs(self): + from netbox.views.generic.object_views import ObjectDeleteView + + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + for _ in range(50) + ]) + + view = ObjectDeleteView() + view.queryset = ScriptModule.objects.all() + dependent_objects = view._get_dependent_objects(module) + + # Single dict returned (not a tuple); jobs represented as a CountOnly. + self.assertIsInstance(dependent_objects, dict) + self.assertIn(Job, dependent_objects) + self.assertIsInstance(dependent_objects[Job], CountOnly) + self.assertEqual(len(dependent_objects[Job]), 50) + + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_confirm_page_renders_job_count(self): + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + for _ in range(50) + ]) + + # ScriptModule is a proxy over core.ManagedFile, so the delete view requires the + # concrete model's permission (core.delete_managedfile), not extras.delete_scriptmodule. + self.add_permissions('core.delete_managedfile') + url = reverse('extras:scriptmodule_delete', kwargs={'pk': module.pk}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + # Assert on the rendered context, not brittle HTML substrings: Job is present in + # dependent_objects as a CountOnly reporting the true count, so the confirmation page + # renders it as a summarized (non-expandable) row without materializing 50 Job rows. + dependent_objects = response.context['dependent_objects'] + self.assertIn(Job, dependent_objects) + self.assertIsInstance(dependent_objects[Job], CountOnly) + self.assertEqual(len(dependent_objects[Job]), 50) + self.assertTrue(dependent_objects[Job].count_only) diff --git a/netbox/netbox/constants.py b/netbox/netbox/constants.py index 037059583..fddce0ad9 100644 --- a/netbox/netbox/constants.py +++ b/netbox/netbox/constants.py @@ -74,3 +74,8 @@ CENSOR_TOKEN_CHANGED = '***CHANGED***' # Placeholder text for empty tables EMPTY_TABLE_TEXT = 'No results found' + +# Batch size for deleting a JobsMixin object's associated jobs during cascade deletion. +# Kept small because each Job carries potentially large data/log_entries payloads and +# cannot be fast-deleted (a global pre_delete receiver forces per-instance signals). See #22812. +JOB_DELETE_BATCH_SIZE = 100 diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index e87321ae3..0d9aa2c19 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -8,6 +8,62 @@ from django.utils.translation import gettext as _ logger = logging.getLogger("netbox.models.deletion") +class CountOnly: + """ + A stand-in for a list of dependent instances that reports a count without holding any + instances. Used on the delete-confirmation page for high-cardinality relations (e.g. a + JobsMixin object's jobs) which we deliberately do not materialize (see #22812). It is a + lenient, empty iterable: `len()` returns the true row count, but iterating yields nothing, + so it slots into the same `{model: }` mapping as real instance lists and renders + as a non-expandable row. + """ + # Template flag: distinguishes a count-only entry (no instances to list) from a real list, + # so the confirmation page can render it without an expand/collapse affordance. + count_only = True + + def __init__(self, count): + self.count = count + + def __len__(self): + return self.count + + def __iter__(self): + return iter(()) + + +class ConfirmCollector(Collector): + """ + A display-only Collector used to enumerate the objects that would be deleted along with a + given object, for rendering the delete confirmation page. It behaves like Django's stock + Collector (preserving the full FK cascade graph and its ProtectedError/RestrictedError + behavior) except that it does not descend into the `jobs` GenericRelation. A JobsMixin + object can accumulate thousands of Jobs, each carrying large data/log_entries payloads; + materializing them all just to render a confirmation page can exhaust memory (see #22812). + Instead, the related Jobs are counted and recorded in `generic_relation_counts`. + """ + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.generic_relation_counts = {} + + def collect(self, objs, source=None, *args, **kwargs): + """ + Override collect() to count the `jobs` GenericRelation rather than descend into it. + + Django's Collector offers no per-relation skip hook, so we intercept the one call it + makes when cascading into a GenericRelation: collect(sub_objs, source=model, ...), where + `sub_objs` is a queryset of the related model. When that model is Job, we count the rows + instead of collecting (and thus instantiating) them, and forward every other call to the + stock implementation untouched. A directly-deleted Job (top-level call, source=None) + still collects normally. + """ + from core.models import Job + + if source is not None and getattr(objs, 'model', None) is Job: + self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + objs.count() + return None + return super().collect(objs, source=source, *args, **kwargs) + + class CustomCollector(Collector): """ Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions. diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 1556a22a1..8755f7eaf 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -5,7 +5,7 @@ from functools import cached_property from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.contenttypes.models import ContentType from django.core.validators import ValidationError -from django.db import models +from django.db import models, router, transaction from django.db.models import Q from django.utils import timezone from django.utils.translation import gettext_lazy as _ @@ -17,7 +17,7 @@ from extras.constants import CUSTOMFIELD_EMPTY_VALUES from extras.managers import NetBoxTaggableManager, NetBoxTaggableManagerField from extras.utils import is_taggable from netbox.config import get_config -from netbox.constants import CORE_APPS +from netbox.constants import CORE_APPS, JOB_DELETE_BATCH_SIZE from netbox.models.deletion import DeleteMixin from netbox.plugins import PluginConfig from netbox.registry import registry @@ -463,6 +463,24 @@ class JobsMixin(models.Model): class Meta: abstract = True + def delete(self, *args, **kwargs): + from core.models import Job + + # Delete associated jobs in batches so the cascade never has to load thousands of + # Job rows (each carrying potentially large data/log_entries payloads) into memory + # at once. Wrapped in a transaction so that a failure in the parent delete rolls the + # job deletions back as well. As with the prior cascade behavior, this bulk delete does + # not invoke Job.delete() and therefore does not cancel the backing RQ job. See #22812. + using = router.db_for_write(self.__class__, instance=self) + with transaction.atomic(using=using): + job_pks = self.jobs.order_by('pk').values_list('pk', flat=True) + # Re-slice the queryset each iteration: it re-queries after each batch delete, so the + # remaining set shrinks and the loop terminates (do not hoist this into a cursor). + while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]): + Job.objects.filter(pk__in=pks).delete() + return super().delete(*args, **kwargs) + delete.alters_data = True + def get_latest_jobs(self): """ Return a list of the most recent jobs for this instance. diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 3e9690030..e815a45cb 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -4,7 +4,6 @@ from collections import defaultdict from django.contrib import messages from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError -from django.db.models.deletion import Collector from django.http import HttpResponse from django.shortcuts import redirect, render from django.urls import reverse @@ -13,6 +12,7 @@ from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ from core.signals import clear_events +from netbox.models.deletion import ConfirmCollector, CountOnly from netbox.object_actions import BulkDelete, BulkEdit, CloneObject, DeleteObject, EditObject from utilities.error_handlers import handle_protectederror from utilities.exceptions import AbortRequest, PermissionsViolation @@ -385,14 +385,19 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): def _get_dependent_objects(self, obj): """ - Returns a dictionary mapping of dependent objects (organized by model) which will be deleted as a result of - deleting the requested object. + Returns a dictionary mapping each dependent model to the objects (of that model) which will + be deleted as a result of deleting the requested object. + + Values are normally a list of instances. For high-cardinality relations that we do not + materialize to avoid excessive memory use (currently a JobsMixin object's jobs, see + #22812), the value is a `CountOnly` — a lenient empty iterable whose `len()` is the true + row count, so it renders as a non-expandable row alongside the itemized relations. Args: obj: The object to return dependent objects for """ using = router.db_for_write(obj._meta.model) - collector = Collector(using=using) + collector = ConfirmCollector(using=using) collector.collect([obj]) # Compile a mapping of models to instances @@ -406,7 +411,13 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): continue dependent_objects[model].append(instances) - return dict(dependent_objects) + # Add count-only entries for relations the collector enumerated by count rather than by + # instance (e.g. jobs), so they render as non-expandable rows in the same mapping. + dependent_objects = dict(dependent_objects) + for model, count in collector.generic_relation_counts.items(): + dependent_objects[model] = CountOnly(count) + + return dependent_objects def _handle_protected_objects(self, obj, protected_objects, request, exc): """ diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html index eb267e44c..8f24816a4 100644 --- a/netbox/templates/htmx/delete_form.html +++ b/netbox/templates/htmx/delete_form.html @@ -24,31 +24,38 @@

{% for model, instances in dependent_objects.items %} -
-

- -

-
-
-
- {% for instance in instances %} - {% with url=instance.get_absolute_url %} - {{ instance }} - {% endwith %} - {% endfor %} + {% with object_count=instances|length %} +
+

+ {# High-cardinality relations (e.g. jobs) are summarized by count and are not #} + {# expandable, since their instances are intentionally not loaded (see #22812). #} + {% if instances.count_only %} + + {% else %} + + {% endif %} +

+ {% if not instances.count_only %} +
+
+
+ {% for instance in instances %} + {% with url=instance.get_absolute_url %} + {{ instance }} + {% endwith %} + {% endfor %} +
+
-
+ {% endif %}
-
+ {% endwith %} {% endfor %}
{% endif %} From 280e32fcc95a2dfb9a51f307e9a0b6b6e9388433 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 05:54:34 +0000 Subject: [PATCH 07/76] Update source translation strings --- netbox/translations/en/LC_MESSAGES/django.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netbox/translations/en/LC_MESSAGES/django.po b/netbox/translations/en/LC_MESSAGES/django.po index 5c63ea216..0c190cdfa 100644 --- a/netbox/translations/en/LC_MESSAGES/django.po +++ b/netbox/translations/en/LC_MESSAGES/django.po @@ -8,7 +8,7 @@ msgid "" msgstr "" "Project-Id-Version: PACKAGE VERSION\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2026-08-04 05:56+0000\n" +"POT-Creation-Date: 2026-08-05 05:54+0000\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" @@ -8305,7 +8305,7 @@ msgstr "" msgid "The rendered template output." msgstr "" -#: netbox/extras/api/serializers_/customfields.py:79 +#: netbox/extras/api/serializers_/customfields.py:80 msgid "Changing the type of custom fields is not supported." msgstr "" From 75ceb5575450a06b93d6f92abf89672dab80298f Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Wed, 5 Aug 2026 18:01:38 +0200 Subject: [PATCH 08/76] fix(extras): Honor Script defaults when triggered by Event Rules Scripts triggered by Event Rules now respect notifications_default and job_timeout from script Meta class. Updates documentation to clarify this behavior and adds regression test coverage. Fixes #22852 --- docs/customization/custom-scripts.md | 8 +- netbox/extras/events.py | 2 + .../extras/management/commands/runscript.py | 2 + netbox/extras/tests/test_event_rules.py | 77 ++++++++++++++++++- .../extras/tests/test_management_commands.py | 29 ++++--- 5 files changed, 98 insertions(+), 20 deletions(-) diff --git a/docs/customization/custom-scripts.md b/docs/customization/custom-scripts.md index e18d7f700..6ebac740a 100644 --- a/docs/customization/custom-scripts.md +++ b/docs/customization/custom-scripts.md @@ -108,7 +108,7 @@ class MyScript(Script): ### `commit_default` -The checkbox to commit database changes when executing a script is checked by default. Set `commit_default` to False under the script's Meta class to leave this option unchecked by default. +The checkbox to commit database changes when executing a script is checked by default. Set `commit_default` to False under the script's Meta class to leave this option unchecked by default. This setting controls only the initial state of the execution form. ```python commit_default = False @@ -120,7 +120,9 @@ By default, a script can be scheduled for execution at a later time. Setting `sc ### `notifications_default` -By default, a notification is generated for the requesting user each time a script finishes running. This attribute sets the initial value for the notifications field when running a script. Valid values are `always` (default), `on_failure`, and `never`. +By default, a notification is generated for the user associated with the script's job each time the script finishes running. This attribute sets the initial value for the notifications field when running a script. Valid values are `always` (default), `on_failure`, and `never`. + +Scripts run from an event rule or the `runscript` management command use this value as their notification policy. For an event rule, the notification goes to the user associated with the triggering event, if there is one. ```python notifications_default = 'on_failure' @@ -134,7 +136,7 @@ notifications_default = 'on_failure' ### `job_timeout` -Set the maximum allowed runtime for the script. If not set, `RQ_DEFAULT_TIMEOUT` will be used. +Set the maximum allowed runtime for the script. If not set, `RQ_DEFAULT_TIMEOUT` will be used. Scripts run from an event rule use this value as their execution timeout. ## Accessing Request Data diff --git a/netbox/extras/events.py b/netbox/extras/events.py index c01e02ed5..e4740779e 100644 --- a/netbox/extras/events.py +++ b/netbox/extras/events.py @@ -253,6 +253,8 @@ def process_event_rules(event_rules, object_type, event): 'name': script.name, 'user': event['user'], 'data': event_data, + 'notifications': script.notifications_default, + 'job_timeout': script.job_timeout, } if 'snapshots' in event: params['snapshots'] = event['snapshots'] diff --git a/netbox/extras/management/commands/runscript.py b/netbox/extras/management/commands/runscript.py index ef3a46fac..1bc9ef958 100644 --- a/netbox/extras/management/commands/runscript.py +++ b/netbox/extras/management/commands/runscript.py @@ -85,6 +85,7 @@ class Command(BaseCommand): form.cleaned_data.pop('_schedule_at') form.cleaned_data.pop('_interval') form.cleaned_data.pop('_commit') + notifications = form.cleaned_data.pop('_notifications') # Execute the script. job = ScriptJob.enqueue( @@ -92,6 +93,7 @@ class Command(BaseCommand): user=user, immediate=True, data=form.cleaned_data, + notifications=notifications, request=NetBoxFakeRequest({ 'META': {}, 'COOKIES': {}, diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 98e6e4b81..d75bf5bf6 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -14,14 +14,14 @@ from PIL import Image from requests import Session from rest_framework import status -from core.choices import ManagedFileRootPathChoices +from core.choices import JobNotificationChoices, ManagedFileRootPathChoices from core.events import * from core.models import Job, ObjectType from dcim.choices import SiteStatusChoices from dcim.models import DeviceType, Interface, Manufacturer, Site from extras.choices import EventRuleActionChoices from extras.events import enqueue_event, flush_events, serialize_for_event -from extras.models import EventRule, Script, ScriptModule, Tag, Webhook +from extras.models import EventRule, Notification, Script, ScriptModule, Tag, Webhook from extras.scripts import Script as ScriptBase from extras.signals import process_job_end_event_rules from extras.webhooks import generate_signature, send_webhook @@ -756,6 +756,79 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase): self.assertEqual(script_job.status, "completed") self.assertEqual(script_job.data.get('output', ''), "finished successfully") + @tag('regression') # Issue #22852 + def test_eventrule_script_action_honors_script_defaults(self): + """A script run from an event rule uses the notification policy and job timeout from its Meta class.""" + class DummyScript(ScriptBase): + class Meta: + name = 'Dummy Defaults Script' + notifications_default = JobNotificationChoices.NOTIFICATION_ON_FAILURE + job_timeout = 600 + + def run(self, data, commit=True): + return 'finished successfully' + + dummy_script = DummyScript() + + with patch.object(ScriptModule, 'sync_classes'): + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path='dummy_defaults_script.py', + ) + script = Script.objects.create( + module=module, + name=dummy_script.name, + is_executable=True, + ) + + event_rule = EventRule.objects.create( + name='Test Script Defaults Event Rule', + event_types=[OBJECT_CREATED], + action_type=EventRuleActionChoices.SCRIPT, + action_object_type=ObjectType.objects.get_for_model(Script), + action_object_id=script.pk, + ) + event_rule.object_types.set([ObjectType.objects.get_for_model(DeviceType)]) + + manufacturer = Manufacturer.objects.create(name='Test Manufacturer', slug='test-manufacturer') + self.add_permissions('dcim.add_devicetype') + + with patch.object(Script, 'python_class') as mock: + mock.return_value = dummy_script + with self.captureOnCommitCallbacks(execute=True): + response = self.client.post( + reverse('dcim-api:devicetype-list'), + { + 'manufacturer': manufacturer.pk, + 'model': 'Test DeviceType', + 'slug': 'test-devicetype', + }, + format='json', + **self.header, + ) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + + self.assertEqual(self.queue.count, 1) + self.assertEqual(self.queue.jobs[0].timeout, 600) + script_job = Job.objects.get(name=dummy_script.name) + self.assertEqual(script_job.notifications, JobNotificationChoices.NOTIFICATION_ON_FAILURE) + + # silence rqworker (cleaner output) and trigger job execution + rq_logger = logging.getLogger('rq.worker') + self.addCleanup(rq_logger.setLevel, rq_logger.level) + rq_logger.setLevel(logging.ERROR) + self.run_rq_jobs('default') + + script_job.refresh_from_db() + self.assertEqual(script_job.status, "completed") + self.assertFalse( + Notification.objects.filter( + user=self.user, + object_type=ObjectType.objects.get_for_model(Job), + object_id=script_job.pk, + ).exists() + ) + @tag('regression') def test_eventrule_webhook_action_with_object_image_files(self): """ diff --git a/netbox/extras/tests/test_management_commands.py b/netbox/extras/tests/test_management_commands.py index 3372de375..3f46f413b 100644 --- a/netbox/extras/tests/test_management_commands.py +++ b/netbox/extras/tests/test_management_commands.py @@ -7,11 +7,13 @@ from django.core.management import call_command from django.core.management.base import CommandError from django.test import TestCase +from core.choices import JobNotificationChoices from dcim.choices import InterfaceTypeChoices from dcim.models import Device, DeviceRole, DeviceType, Interface, Manufacturer, Site from extras.management.commands import renaturalize, webhook_receiver from extras.management.commands.webhook_receiver import WebhookHandler from extras.models import ImageAttachment +from extras.scripts import Script, StringVar from extras.tests.test_models import OverwriteStyleMemoryStorage, UnreadableSizeMemoryStorage from users.models import User from utilities.fields import NaturalOrderingField @@ -255,20 +257,14 @@ class RunScriptTestCase(TestCase): ) def test_enqueues_script_job(self): - class TestScript: - full_name = 'test.Script' + class TestScript(Script): + value = StringVar() - def as_form(self, data, files): - form = MagicMock() - form.is_valid.return_value = True - form.cleaned_data = { - '_schedule_at': None, - '_interval': None, - '_commit': None, - 'name': data['name'], - } - form.errors.get_json_data.return_value = {} - return form + class Meta: + notifications_default = JobNotificationChoices.NOTIFICATION_ON_FAILURE + + def run(self, data, commit): + return None script_obj = SimpleNamespace(python_class=TestScript) job = SimpleNamespace(duration='0 seconds') @@ -288,7 +284,7 @@ class RunScriptTestCase(TestCase): 'runscript', 'test.Script', user='admin', - data='{"name": "test"}', + data='{"value": "test"}', stdout=StringIO(), ) @@ -298,8 +294,9 @@ class RunScriptTestCase(TestCase): self.assertEqual(kwargs['instance'], script_obj) self.assertEqual(kwargs['user'], self.user) self.assertTrue(kwargs['immediate']) - self.assertEqual(kwargs['data'], {'name': 'test'}) + self.assertEqual(kwargs['data'], {'value': 'test'}) self.assertFalse(kwargs['commit']) + self.assertEqual(kwargs['notifications'], JobNotificationChoices.NOTIFICATION_ON_FAILURE) def test_invalid_script_data_raises_error_without_enqueueing_job(self): class TestScript: @@ -351,6 +348,7 @@ class RunScriptTestCase(TestCase): '_schedule_at': None, '_interval': None, '_commit': None, + '_notifications': JobNotificationChoices.NOTIFICATION_ALWAYS, } form.errors.get_json_data.return_value = {} return form @@ -391,6 +389,7 @@ class RunScriptTestCase(TestCase): '_schedule_at': None, '_interval': None, '_commit': None, + '_notifications': JobNotificationChoices.NOTIFICATION_ALWAYS, } form.errors.get_json_data.return_value = {} return form From f2923f4ce4c2a741be823fd3d2f2f345b0ef1bb7 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 13:56:01 -0500 Subject: [PATCH 09/76] Fixes #22812: Batch child-script job deletion when deleting a ScriptModule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a Script via the UI is only possible by deleting its parent ScriptModule (no Script delete view exists). That cascades to the child Script rows, and the collector materialized every one of those Scripts' jobs — the memory blowup, which scales with jobs-per-script. JobsMixin.delete() only batched the deleted object's own jobs, and a ScriptModule has none; the jobs live on its child Scripts. Extract the chunked job-deletion loop from JobsMixin.delete() into a shared batch_delete_jobs() helper, and add a ScriptModule.delete() override that batch-deletes its child Scripts' jobs (in a single queryset keyed on the script PKs, no per-script loop) before delegating to the cascade. This bounds peak memory to one batch regardless of how many jobs the module's scripts hold. --- netbox/extras/models/scripts.py | 27 ++++++++++++++-- netbox/extras/tests/test_scripts_deletion.py | 24 ++++++++++++++ netbox/netbox/models/features.py | 34 +++++++++++++------- 3 files changed, 71 insertions(+), 14 deletions(-) diff --git a/netbox/extras/models/scripts.py b/netbox/extras/models/scripts.py index 02f3902d1..d9a2ca28c 100644 --- a/netbox/extras/models/scripts.py +++ b/netbox/extras/models/scripts.py @@ -3,7 +3,8 @@ import logging from functools import cached_property from django.contrib.contenttypes.fields import GenericRelation -from django.db import models +from django.contrib.contenttypes.models import ContentType +from django.db import models, router, transaction from django.db.models import Q from django.urls import reverse from django.utils.translation import gettext_lazy as _ @@ -11,7 +12,7 @@ from django.utils.translation import gettext_lazy as _ from core.choices import ManagedFileRootPathChoices from core.models import ManagedFile from extras.utils import is_script -from netbox.models.features import EventRulesMixin, JobsMixin +from netbox.models.features import EventRulesMixin, JobsMixin, batch_delete_jobs from utilities.querysets import RestrictedQuerySet from .mixins import PythonModuleMixin @@ -119,6 +120,28 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile): def __str__(self): return self.python_name + def delete(self, *args, **kwargs): + # Job is imported here rather than at module level to avoid a circular import + # (core.models.jobs -> core.signals -> extras.events -> extras.models -> this module). + from core.models import Job + + # Deleting a ScriptModule cascades (via the Script.module FK) to its child Scripts, and + # Django's collector would materialize every one of those Scripts' Jobs to delete them. + # A module's scripts can accumulate thousands of jobs, exhausting memory. Batch-delete + # the child Scripts' jobs up front, in a single queryset (no per-script loop), before + # delegating to the cascade. Wrapped in a transaction so a failure in the parent delete + # rolls these deletions back as well. See #22812. + using = router.db_for_write(self.__class__, instance=self) + with transaction.atomic(using=using): + script_type = ContentType.objects.get_for_model(Script, for_concrete_model=False) + child_jobs = Job.objects.filter( + object_type=script_type, + object_id__in=self.scripts.values_list('pk', flat=True), + ) + batch_delete_jobs(child_jobs) + return super().delete(*args, **kwargs) + delete.alters_data = True + @property def ordered_scripts(self): script_objects = {s.name: s for s in self.scripts.all()} diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 9aaf11e0c..5b928bd0d 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -90,6 +90,30 @@ class ScriptDeletionTestCase(TestCase): self.assertFalse(Script.objects.filter(pk=script.pk).exists()) self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + def test_delete_scriptmodule_batches_child_script_jobs(self): + # The reporter's actual path: a script is only removable via the UI by deleting its + # ScriptModule. The module's delete must batch the child Script's jobs. + module, script = self._create_script() + self._add_jobs(script, 5) + + job_delete_calls = [] + original_delete = QuerySet.delete + + def counting_delete(qs, *args, **kwargs): + if qs.model is Job: + job_delete_calls.append(len(qs)) + return original_delete(qs, *args, **kwargs) + + with mock.patch('netbox.models.features.JOB_DELETE_BATCH_SIZE', 2): + with mock.patch.object(QuerySet, 'delete', counting_delete): + module.delete() + + # 5 child-script jobs at a batch size of 2 => three batched deletes (2, 2, 1). The module + # has no jobs of its own, so JobsMixin.delete adds no further Job deletes. + self.assertEqual(job_delete_calls, [2, 2, 1]) + self.assertFalse(Script.objects.filter(pk=script.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + def test_delete_datasource_deletes_jobs(self): datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test') self._add_jobs(datasource, 100) diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 8755f7eaf..f608da50c 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -43,6 +43,7 @@ __all__ = ( 'NotificationsMixin', 'SyncedDataMixin', 'TagsMixin', + 'batch_delete_jobs', 'get_model_features', 'has_feature', 'model_is_public', @@ -449,6 +450,23 @@ class NotificationsMixin(models.Model): abstract = True +def batch_delete_jobs(job_queryset): + """ + Delete the Jobs in `job_queryset` in JOB_DELETE_BATCH_SIZE chunks, so the caller never has + to load thousands of Job rows (each carrying potentially large data/log_entries payloads) + into memory at once. Callers are responsible for wrapping this in a transaction. As with the + prior cascade behavior, this bulk delete does not invoke Job.delete() and therefore does not + cancel the backing RQ job. See #22812. + """ + from core.models import Job + + job_pks = job_queryset.order_by('pk').values_list('pk', flat=True) + # Re-slice the queryset each iteration: it re-queries after each batch delete, so the + # remaining set shrinks and the loop terminates (do not hoist this into a cursor). + while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]): + Job.objects.filter(pk__in=pks).delete() + + class JobsMixin(models.Model): """ Enables support for job results. @@ -464,20 +482,12 @@ class JobsMixin(models.Model): abstract = True def delete(self, *args, **kwargs): - from core.models import Job - - # Delete associated jobs in batches so the cascade never has to load thousands of - # Job rows (each carrying potentially large data/log_entries payloads) into memory - # at once. Wrapped in a transaction so that a failure in the parent delete rolls the - # job deletions back as well. As with the prior cascade behavior, this bulk delete does - # not invoke Job.delete() and therefore does not cancel the backing RQ job. See #22812. + # Delete associated jobs in batches so the cascade never has to load thousands of Job + # rows into memory at once. Wrapped in a transaction so that a failure in the parent + # delete rolls the job deletions back as well. See #22812. using = router.db_for_write(self.__class__, instance=self) with transaction.atomic(using=using): - job_pks = self.jobs.order_by('pk').values_list('pk', flat=True) - # Re-slice the queryset each iteration: it re-queries after each batch delete, so the - # remaining set shrinks and the loop terminates (do not hoist this into a cursor). - while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]): - Job.objects.filter(pk__in=pks).delete() + batch_delete_jobs(self.jobs) return super().delete(*args, **kwargs) delete.alters_data = True From be69c55a990ba99fbcad774e85b7fdb562b8737e Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 13:59:02 -0500 Subject: [PATCH 10/76] Fixes #22812: Don't show a spurious "0 jobs" row for jobless objects Django's Collector calls into the jobs GenericRelation branch unconditionally, so ConfirmCollector recorded a zero count for objects with no jobs. _get_dependent_objects then added a CountOnly(0), and the delete-confirmation page rendered "The following objects will be deleted as a result of this action." plus a "0 jobs" row for every jobless JobsMixin object. Only record a count when there are actually jobs. --- netbox/extras/tests/test_scripts_deletion.py | 25 ++++++++++++++++++++ netbox/netbox/models/deletion.py | 7 +++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 5b928bd0d..1d5c92581 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -199,6 +199,16 @@ class ConfirmCollectorTestCase(TestCase): self.assertEqual(list(wrapper), []) self.assertTrue(wrapper.count_only) + def test_confirm_collector_omits_jobs_when_none(self): + # A jobless object must not record a zero count, or the confirmation page would show a + # spurious "0 jobs" row (#22812 regression). + datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test') + + collector = ConfirmCollector(using=router.db_for_write(DataSource)) + collector.collect([datasource]) + + self.assertNotIn(Job, collector.generic_relation_counts) + class ObjectDeleteViewCountsTestCase(ViewTestCase): """ @@ -258,3 +268,18 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): self.assertIsInstance(dependent_objects[Job], CountOnly) self.assertEqual(len(dependent_objects[Job]), 50) self.assertTrue(dependent_objects[Job].count_only) + + def test_get_dependent_objects_omits_jobs_when_none(self): + from netbox.views.generic.object_views import ObjectDeleteView + + # A module with no jobs must not produce a CountOnly(0) entry (#22812 regression). + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + + view = ObjectDeleteView() + view.queryset = ScriptModule.objects.all() + dependent_objects = view._get_dependent_objects(module) + + self.assertNotIn(Job, dependent_objects) diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 0d9aa2c19..a903b7dff 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -59,7 +59,12 @@ class ConfirmCollector(Collector): from core.models import Job if source is not None and getattr(objs, 'model', None) is Job: - self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + objs.count() + # Django calls this branch for the jobs relation even when there are none; only record + # a count when there are actually jobs, so jobless objects don't get a spurious + # "0 jobs" row on the delete-confirmation page. + count = objs.count() + if count: + self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + count return None return super().collect(objs, source=source, *args, **kwargs) From 7d46c995f79637f0361fb230a4d8c746d6deb5df Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 14:04:19 -0500 Subject: [PATCH 11/76] Fixes #22812: Defer large Job payload fields during batched deletion The batched job delete can't fast-delete (a global pre_delete receiver forces per-instance signals), so each batch still instantiates its Job rows. Load only the PK via only('pk') so those instances don't pull the large data/log_entries payloads, cutting the resident set per batch. Also drop a dead `no-toggle` CSS class from the delete-confirmation template (it is defined nowhere and, under Tabler, has no effect) and use JobStatusChoices.STATUS_COMPLETED in the tests instead of a string literal. --- netbox/extras/tests/test_scripts_deletion.py | 16 +++++++++++----- netbox/netbox/models/features.py | 5 ++++- netbox/templates/htmx/delete_form.html | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 1d5c92581..f6d9bad48 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -7,7 +7,7 @@ from django.db.models import QuerySet from django.test import TestCase, override_settings from django.urls import reverse -from core.choices import ManagedFileRootPathChoices +from core.choices import JobStatusChoices, ManagedFileRootPathChoices from core.models import DataSource, Job from extras.models import Script, ScriptModule from extras.validators import CustomValidator @@ -43,7 +43,7 @@ class ScriptDeletionTestCase(TestCase): object_type=object_type, object_id=obj.pk, name='testjob', - status='completed', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), data={'output': 'x' * 50}, ) @@ -164,7 +164,7 @@ class ConfirmCollectorTestCase(TestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', + Job(object_type=ct, object_id=script.pk, name='j', status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), data={'output': 'x' * 50}) for _ in range(count) ]) @@ -226,7 +226,10 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + Job( + object_type=ct, object_id=script.pk, name='j', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), + ) for _ in range(50) ]) @@ -249,7 +252,10 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + Job( + object_type=ct, object_id=script.pk, name='j', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), + ) for _ in range(50) ]) diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index f608da50c..4f1d5b684 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -464,7 +464,10 @@ def batch_delete_jobs(job_queryset): # Re-slice the queryset each iteration: it re-queries after each batch delete, so the # remaining set shrinks and the loop terminates (do not hoist this into a cursor). while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]): - Job.objects.filter(pk__in=pks).delete() + # only('pk'): the batch still can't fast-delete (a global pre_delete receiver forces + # per-instance signals), so each Job in the batch is instantiated. Loading just the PK + # avoids pulling the large data/log_entries payloads into those instances. + Job.objects.filter(pk__in=pks).only('pk').delete() class JobsMixin(models.Model): diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html index 8f24816a4..c07e83d0a 100644 --- a/netbox/templates/htmx/delete_form.html +++ b/netbox/templates/htmx/delete_form.html @@ -30,7 +30,7 @@ {# High-cardinality relations (e.g. jobs) are summarized by count and are not #} {# expandable, since their instances are intentionally not loaded (see #22812). #} {% if instances.count_only %} -