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 %}