From 7323668dd0a5a1452a7e94c2f43c9bce60383755 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 31 Oct 2023 08:34:57 -0400 Subject: [PATCH 1/6] Closes #13334: Record error message on failed jobs (#14106) --- netbox/core/api/serializers.py | 2 +- netbox/core/jobs.py | 2 +- .../migrations/0006_job_add_error_field.py | 18 ++++++++++++++++++ netbox/core/models/jobs.py | 9 ++++++++- netbox/core/tables/jobs.py | 2 +- netbox/extras/management/commands/runscript.py | 2 +- netbox/extras/reports.py | 6 +++--- netbox/extras/scripts.py | 2 +- netbox/templates/core/job.html | 6 ++++++ 9 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 netbox/core/migrations/0006_job_add_error_field.py diff --git a/netbox/core/api/serializers.py b/netbox/core/api/serializers.py index 0d743d952..4ae426df5 100644 --- a/netbox/core/api/serializers.py +++ b/netbox/core/api/serializers.py @@ -69,5 +69,5 @@ class JobSerializer(BaseModelSerializer): model = Job fields = [ 'id', 'url', 'display', 'object_type', 'object_id', 'name', 'status', 'created', 'scheduled', 'interval', - 'started', 'completed', 'user', 'data', 'job_id', + 'started', 'completed', 'user', 'data', 'error', 'job_id', ] diff --git a/netbox/core/jobs.py b/netbox/core/jobs.py index d25981920..32b546b20 100644 --- a/netbox/core/jobs.py +++ b/netbox/core/jobs.py @@ -25,7 +25,7 @@ def sync_datasource(job, *args, **kwargs): job.terminate() except Exception as e: - job.terminate(status=JobStatusChoices.STATUS_ERRORED) + job.terminate(status=JobStatusChoices.STATUS_ERRORED, error=str(e)) DataSource.objects.filter(pk=datasource.pk).update(status=DataSourceStatusChoices.FAILED) if type(e) in (SyncError, JobTimeoutException): logging.error(e) diff --git a/netbox/core/migrations/0006_job_add_error_field.py b/netbox/core/migrations/0006_job_add_error_field.py new file mode 100644 index 000000000..2927db4c4 --- /dev/null +++ b/netbox/core/migrations/0006_job_add_error_field.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.6 on 2023-10-23 20:28 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0005_job_created_auto_now'), + ] + + operations = [ + migrations.AddField( + model_name='job', + name='error', + field=models.TextField(blank=True, editable=False), + ), + ] diff --git a/netbox/core/models/jobs.py b/netbox/core/models/jobs.py index 61b0e64fa..4e9a93bfb 100644 --- a/netbox/core/models/jobs.py +++ b/netbox/core/models/jobs.py @@ -92,6 +92,11 @@ class Job(models.Model): null=True, blank=True ) + error = models.TextField( + verbose_name=_('error'), + editable=False, + blank=True + ) job_id = models.UUIDField( verbose_name=_('job ID'), unique=True @@ -158,7 +163,7 @@ class Job(models.Model): # Handle webhooks self.trigger_webhooks(event=EVENT_JOB_START) - def terminate(self, status=JobStatusChoices.STATUS_COMPLETED): + def terminate(self, status=JobStatusChoices.STATUS_COMPLETED, error=None): """ Mark the job as completed, optionally specifying a particular termination status. """ @@ -168,6 +173,8 @@ class Job(models.Model): # Mark the job as completed self.status = status + if error: + self.error = error self.completed = timezone.now() self.save() diff --git a/netbox/core/tables/jobs.py b/netbox/core/tables/jobs.py index 32ca67f7f..3388aee19 100644 --- a/netbox/core/tables/jobs.py +++ b/netbox/core/tables/jobs.py @@ -47,7 +47,7 @@ class JobTable(NetBoxTable): model = Job fields = ( 'pk', 'id', 'object_type', 'object', 'name', 'status', 'created', 'scheduled', 'interval', 'started', - 'completed', 'user', 'job_id', + 'completed', 'user', 'error', 'job_id', ) default_columns = ( 'pk', 'id', 'object_type', 'object', 'name', 'status', 'created', 'started', 'completed', 'user', diff --git a/netbox/extras/management/commands/runscript.py b/netbox/extras/management/commands/runscript.py index d9a9f41ae..3cf70281c 100644 --- a/netbox/extras/management/commands/runscript.py +++ b/netbox/extras/management/commands/runscript.py @@ -59,7 +59,7 @@ class Command(BaseCommand): logger.error(f"Exception raised during script execution: {e}") clear_webhooks.send(request) job.data = ScriptOutputSerializer(script).data - job.terminate(status=JobStatusChoices.STATUS_ERRORED) + job.terminate(status=JobStatusChoices.STATUS_ERRORED, error=str(e)) logger.info(f"Script completed in {job.duration}") diff --git a/netbox/extras/reports.py b/netbox/extras/reports.py index cc279a49a..c8a13fe15 100644 --- a/netbox/extras/reports.py +++ b/netbox/extras/reports.py @@ -40,8 +40,8 @@ def run_report(job, *args, **kwargs): try: report.run(job) - except Exception: - job.terminate(status=JobStatusChoices.STATUS_ERRORED) + except Exception as e: + job.terminate(status=JobStatusChoices.STATUS_ERRORED, error=str(e)) logging.error(f"Error during execution of report {job.name}") finally: # Schedule the next job if an interval has been set @@ -230,7 +230,7 @@ class Report(object): stacktrace = traceback.format_exc() self.log_failure(None, f"An exception occurred: {type(e).__name__}: {e} 
{stacktrace}
") logger.error(f"Exception raised during report execution: {e}") - job.terminate(status=JobStatusChoices.STATUS_ERRORED) + job.terminate(status=JobStatusChoices.STATUS_ERRORED, error=str(e)) # Perform any post-run tasks self.post_run() diff --git a/netbox/extras/scripts.py b/netbox/extras/scripts.py index e93326ddc..df75200e6 100644 --- a/netbox/extras/scripts.py +++ b/netbox/extras/scripts.py @@ -519,7 +519,7 @@ def run_script(data, request, job, commit=True, **kwargs): logger.error(f"Exception raised during script execution: {e}") script.log_info("Database changes have been reverted due to error.") job.data = ScriptOutputSerializer(script).data - job.terminate(status=JobStatusChoices.STATUS_ERRORED) + job.terminate(status=JobStatusChoices.STATUS_ERRORED, error=str(e)) clear_webhooks.send(request) logger.info(f"Script completed in {job.duration}") diff --git a/netbox/templates/core/job.html b/netbox/templates/core/job.html index 1fe3862cd..deb651739 100644 --- a/netbox/templates/core/job.html +++ b/netbox/templates/core/job.html @@ -35,6 +35,12 @@ {% trans "Status" %} {% badge object.get_status_display object.get_status_color %} + {% if object.error %} + + {% trans "Error" %} + {{ object.error }} + + {% endif %} {% trans "Created By" %} {{ object.user|placeholder }} From 77208bf5f3af6e7f2500baf676a127f07fea319a Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 31 Oct 2023 08:41:24 -0400 Subject: [PATCH 2/6] Fix migrations --- ...{0006_job_add_error_field.py => 0007_job_add_error_field.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename netbox/core/migrations/{0006_job_add_error_field.py => 0007_job_add_error_field.py} (85%) diff --git a/netbox/core/migrations/0006_job_add_error_field.py b/netbox/core/migrations/0007_job_add_error_field.py similarity index 85% rename from netbox/core/migrations/0006_job_add_error_field.py rename to netbox/core/migrations/0007_job_add_error_field.py index 2927db4c4..e2e173bfd 100644 --- a/netbox/core/migrations/0006_job_add_error_field.py +++ b/netbox/core/migrations/0007_job_add_error_field.py @@ -6,7 +6,7 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ('core', '0005_job_created_auto_now'), + ('core', '0006_datasource_type_remove_choices'), ] operations = [ From c2d1988cb3f74f8a4f7c6005d32caadc69164610 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 1 Nov 2023 11:56:14 -0400 Subject: [PATCH 3/6] Closes #14035: Order global search results of equivalent weight by value (#14140) --- .../migrations/0099_cachedvalue_ordering.py | 17 +++++++++++++++++ netbox/extras/models/search.py | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 netbox/extras/migrations/0099_cachedvalue_ordering.py diff --git a/netbox/extras/migrations/0099_cachedvalue_ordering.py b/netbox/extras/migrations/0099_cachedvalue_ordering.py new file mode 100644 index 000000000..242ffd983 --- /dev/null +++ b/netbox/extras/migrations/0099_cachedvalue_ordering.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.6 on 2023-10-30 14:04 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('extras', '0098_webhook_custom_field_data_webhook_tags'), + ] + + operations = [ + migrations.AlterModelOptions( + name='cachedvalue', + options={'ordering': ('weight', 'object_type', 'value', 'object_id')}, + ), + ] diff --git a/netbox/extras/models/search.py b/netbox/extras/models/search.py index debe4c648..39ff80215 100644 --- a/netbox/extras/models/search.py +++ b/netbox/extras/models/search.py @@ -50,7 +50,7 @@ class CachedValue(models.Model): ) class Meta: - ordering = ('weight', 'object_type', 'object_id') + ordering = ('weight', 'object_type', 'value', 'object_id') verbose_name = _('cached value') verbose_name_plural = _('cached values') From 944008d4753194276908b54dc7f3e0952ec7a4f6 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 1 Nov 2023 13:47:14 -0400 Subject: [PATCH 4/6] Closes #12135: Prevent the deletion of interfaces with children (#14091) * Closes #12135: Prevent the deletion of interfaces with children * Change PROTECT to RESTRICT * Extend handle_protectederror() to also handle RestrictedError * Fix string translation * Update migrations * Support bulk removal of parent interfaces via UI if all children are included * Add support for the bulk deletion of restricted objects via REST API --- docs/models/dcim/interface.md | 3 ++ docs/models/virtualization/vminterface.md | 3 ++ netbox/dcim/api/views.py | 5 ++++ .../0183_protect_child_interfaces.py | 19 ++++++++++++ netbox/dcim/models/device_components.py | 2 +- netbox/dcim/tests/test_api.py | 27 +++++++++++++++++ netbox/dcim/tests/test_views.py | 30 +++++++++++++++++++ netbox/dcim/views.py | 5 ++-- netbox/netbox/api/viewsets/__init__.py | 9 ++++-- netbox/netbox/api/viewsets/mixins.py | 10 +++++-- netbox/netbox/views/generic/bulk_views.py | 17 ++++++----- netbox/netbox/views/generic/object_views.py | 6 ++-- netbox/utilities/error_handlers.py | 20 +++++++++---- netbox/virtualization/api/views.py | 5 ++++ .../0037_protect_child_interfaces.py | 19 ++++++++++++ netbox/virtualization/tests/test_api.py | 26 ++++++++++++++++ netbox/virtualization/tests/test_views.py | 29 ++++++++++++++++++ netbox/virtualization/views.py | 5 ++-- 18 files changed, 214 insertions(+), 26 deletions(-) create mode 100644 netbox/dcim/migrations/0183_protect_child_interfaces.py create mode 100644 netbox/virtualization/migrations/0037_protect_child_interfaces.py diff --git a/docs/models/dcim/interface.md b/docs/models/dcim/interface.md index 42b570964..3667dabd5 100644 --- a/docs/models/dcim/interface.md +++ b/docs/models/dcim/interface.md @@ -77,6 +77,9 @@ If selected, this component will be treated as if a cable has been connected. Virtual interfaces can be bound to a physical parent interface. This is helpful for modeling virtual interfaces which employ encapsulation on a physical interface, such as an 802.1Q VLAN-tagged subinterface. +!!! note + An interface with one or more child interfaces assigned cannot be deleted until all its child interfaces have been deleted or reassigned. + ### Bridged Interface Interfaces can be bridged to other interfaces on a device in two manners: symmetric or grouped. diff --git a/docs/models/virtualization/vminterface.md b/docs/models/virtualization/vminterface.md index 264fb95ba..d923bdd5d 100644 --- a/docs/models/virtualization/vminterface.md +++ b/docs/models/virtualization/vminterface.md @@ -16,6 +16,9 @@ The interface's name. Must be unique to the assigned VM. Identifies the parent interface of a subinterface (e.g. used to employ encapsulation). +!!! note + An interface with one or more child interfaces assigned cannot be deleted until all its child interfaces have been deleted or reassigned. + ### Bridged Interface An interface on the same VM with which this interface is bridged. diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index 80a991736..a3e532f0b 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -24,6 +24,7 @@ from netbox.api.viewsets import NetBoxModelViewSet, MPTTLockedMixin from netbox.api.viewsets.mixins import SequentialBulkCreatesMixin from netbox.constants import NESTED_SERIALIZER_PREFIX from utilities.api import get_serializer_for_model +from utilities.query_functions import CollateAsChar from utilities.utils import count_related from virtualization.models import VirtualMachine from . import serializers @@ -505,6 +506,10 @@ class InterfaceViewSet(PathEndpointMixin, NetBoxModelViewSet): filterset_class = filtersets.InterfaceFilterSet brief_prefetch_fields = ['device'] + def get_bulk_destroy_queryset(self): + # Ensure child interfaces are deleted prior to their parents + return self.get_queryset().order_by('device', 'parent', CollateAsChar('_name')) + class FrontPortViewSet(PassThroughPortMixin, NetBoxModelViewSet): queryset = FrontPort.objects.prefetch_related( diff --git a/netbox/dcim/migrations/0183_protect_child_interfaces.py b/netbox/dcim/migrations/0183_protect_child_interfaces.py new file mode 100644 index 000000000..ca695f4bd --- /dev/null +++ b/netbox/dcim/migrations/0183_protect_child_interfaces.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.6 on 2023-10-20 11:48 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('dcim', '0182_devicetype_exclude_from_utilization'), + ] + + operations = [ + migrations.AlterField( + model_name='interface', + name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='child_interfaces', to='dcim.interface'), + ), + ] diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index 639f8aadb..94568459e 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -537,7 +537,7 @@ class BaseInterface(models.Model): ) parent = models.ForeignKey( to='self', - on_delete=models.SET_NULL, + on_delete=models.RESTRICT, related_name='child_interfaces', null=True, blank=True, diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 1ce362963..d3211a75f 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -1607,6 +1607,33 @@ class InterfaceTest(Mixins.ComponentTraceMixin, APIViewTestCases.APIViewTestCase }, ] + def test_bulk_delete_child_interfaces(self): + interface1 = Interface.objects.get(name='Interface 1') + device = interface1.device + self.add_permissions('dcim.delete_interface') + + # Create a child interface + child = Interface.objects.create( + device=device, + name='Interface 1A', + type=InterfaceTypeChoices.TYPE_VIRTUAL, + parent=interface1 + ) + self.assertEqual(device.interfaces.count(), 4) + + # Attempt to delete only the parent interface + url = self._get_detail_url(interface1) + self.client.delete(url, **self.header) + self.assertEqual(device.interfaces.count(), 4) # Parent was not deleted + + # Attempt to bulk delete parent & child together + data = [ + {"id": interface1.pk}, + {"id": child.pk}, + ] + self.client.delete(self._get_list_url(), data, format='json', **self.header) + self.assertEqual(device.interfaces.count(), 2) # Child & parent were both deleted + class FrontPortTest(APIViewTestCases.APIViewTestCase): model = FrontPort diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index a6981451f..88e0d44f2 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -2531,6 +2531,36 @@ class InterfaceTestCase(ViewTestCases.DeviceComponentViewTestCase): response = self.client.get(reverse('dcim:interface_trace', kwargs={'pk': interface1.pk})) self.assertHttpStatus(response, 200) + def test_bulk_delete_child_interfaces(self): + interface1 = Interface.objects.get(name='Interface 1') + device = interface1.device + self.add_permissions('dcim.delete_interface') + + # Create a child interface + child = Interface.objects.create( + device=device, + name='Interface 1A', + type=InterfaceTypeChoices.TYPE_VIRTUAL, + parent=interface1 + ) + self.assertEqual(device.interfaces.count(), 6) + + # Attempt to delete only the parent interface + data = { + 'confirm': True, + } + self.client.post(self._get_url('delete', interface1), data) + self.assertEqual(device.interfaces.count(), 6) # Parent was not deleted + + # Attempt to bulk delete parent & child together + data = { + 'pk': [interface1.pk, child.pk], + 'confirm': True, + '_confirm': True, # Form button + } + self.client.post(self._get_url('bulk_delete'), data) + self.assertEqual(device.interfaces.count(), 4) # Child & parent were both deleted + class FrontPortTestCase(ViewTestCases.DeviceComponentViewTestCase): model = FrontPort diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 0f5768173..be0d6bcbb 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1,5 +1,4 @@ import traceback -from collections import defaultdict from django.contrib import messages from django.contrib.contenttypes.models import ContentType @@ -26,6 +25,7 @@ from tenancy.views import ObjectContactsView from utilities.forms import ConfirmationForm from utilities.paginator import EnhancedPaginator, get_paginate_count from utilities.permissions import get_permission_for_model +from utilities.query_functions import CollateAsChar from utilities.utils import count_related from utilities.views import GetReturnURLMixin, ObjectPermissionRequiredMixin, ViewTab, register_model_view from virtualization.models import VirtualMachine @@ -2562,7 +2562,8 @@ class InterfaceBulkDisconnectView(BulkDisconnectView): class InterfaceBulkDeleteView(generic.BulkDeleteView): - queryset = Interface.objects.all() + # Ensure child interfaces are deleted prior to their parents + queryset = Interface.objects.order_by('device', 'parent', CollateAsChar('_name')) filterset = filtersets.InterfaceFilterSet table = tables.InterfaceTable diff --git a/netbox/netbox/api/viewsets/__init__.py b/netbox/netbox/api/viewsets/__init__.py index c6794bb61..522bcf77b 100644 --- a/netbox/netbox/api/viewsets/__init__.py +++ b/netbox/netbox/api/viewsets/__init__.py @@ -2,7 +2,7 @@ import logging from django.core.exceptions import ObjectDoesNotExist, PermissionDenied from django.db import transaction -from django.db.models import ProtectedError +from django.db.models import ProtectedError, RestrictedError from django_pglocks import advisory_lock from netbox.constants import ADVISORY_LOCK_KEYS from rest_framework import mixins as drf_mixins @@ -91,8 +91,11 @@ class NetBoxModelViewSet( try: return super().dispatch(request, *args, **kwargs) - except ProtectedError as e: - protected_objects = list(e.protected_objects) + except (ProtectedError, RestrictedError) as e: + if type(e) is ProtectedError: + protected_objects = list(e.protected_objects) + else: + protected_objects = list(e.restricted_objects) msg = f'Unable to delete object. {len(protected_objects)} dependent objects were found: ' msg += ', '.join([f'{obj} ({obj.pk})' for obj in protected_objects]) logger.warning(msg) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index fde486fe9..7b6c00843 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -137,11 +137,14 @@ class BulkUpdateModelMixin: } ] """ + def get_bulk_update_queryset(self): + return self.get_queryset() + def bulk_update(self, request, *args, **kwargs): partial = kwargs.pop('partial', False) serializer = BulkOperationSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) - qs = self.get_queryset().filter( + qs = self.get_bulk_update_queryset().filter( pk__in=[o['id'] for o in serializer.data] ) @@ -184,10 +187,13 @@ class BulkDestroyModelMixin: {"id": 456} ] """ + def get_bulk_destroy_queryset(self): + return self.get_queryset() + def bulk_destroy(self, request, *args, **kwargs): serializer = BulkOperationSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) - qs = self.get_queryset().filter( + qs = self.get_bulk_destroy_queryset().filter( pk__in=[o['id'] for o in serializer.data] ) diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index 676e3f5af..fbe3aa2ba 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -7,7 +7,7 @@ from django.contrib.contenttypes.fields import GenericRel from django.contrib.contenttypes.models import ContentType from django.core.exceptions import FieldDoesNotExist, ObjectDoesNotExist, ValidationError from django.db import transaction, IntegrityError -from django.db.models import ManyToManyField, ProtectedError +from django.db.models import ManyToManyField, ProtectedError, RestrictedError from django.db.models.fields.reverse_related import ManyToManyRel from django.forms import HiddenInput, ModelMultipleChoiceField, MultipleHiddenInput from django.http import HttpResponse @@ -798,14 +798,15 @@ class BulkDeleteView(GetReturnURLMixin, BaseMultiObjectView): queryset = self.queryset.filter(pk__in=pk_list) deleted_count = queryset.count() try: - for obj in queryset: - # Take a snapshot of change-logged models - if hasattr(obj, 'snapshot'): - obj.snapshot() - obj.delete() + with transaction.atomic(): + for obj in queryset: + # Take a snapshot of change-logged models + if hasattr(obj, 'snapshot'): + obj.snapshot() + obj.delete() - except ProtectedError as e: - logger.info("Caught ProtectedError while attempting to delete objects") + except (ProtectedError, RestrictedError) as e: + logger.info(f"Caught {type(e)} while attempting to delete objects") handle_protectederror(queryset, request, e) return redirect(self.get_return_url(request)) diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 99d8ff540..7c737aaf0 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -3,7 +3,7 @@ from copy import deepcopy from django.contrib import messages from django.db import transaction -from django.db.models import ProtectedError +from django.db.models import ProtectedError, RestrictedError from django.shortcuts import redirect, render from django.urls import reverse from django.utils.html import escape @@ -374,8 +374,8 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): try: obj.delete() - except ProtectedError as e: - logger.info("Caught ProtectedError while attempting to delete object") + except (ProtectedError, RestrictedError) as e: + logger.info(f"Caught {type(e)} while attempting to delete objects") handle_protectederror([obj], request, e) return redirect(obj.get_absolute_url()) diff --git a/netbox/utilities/error_handlers.py b/netbox/utilities/error_handlers.py index 1d3bdbafd..9af12ac2e 100644 --- a/netbox/utilities/error_handlers.py +++ b/netbox/utilities/error_handlers.py @@ -1,16 +1,26 @@ from django.contrib import messages +from django.db.models import ProtectedError, RestrictedError from django.utils.html import escape from django.utils.safestring import mark_safe +from django.utils.translation import gettext_lazy as _ def handle_protectederror(obj_list, request, e): """ - Generate a user-friendly error message in response to a ProtectedError exception. + Generate a user-friendly error message in response to a ProtectedError or RestrictedError exception. """ - protected_objects = list(e.protected_objects) - protected_count = len(protected_objects) if len(protected_objects) <= 50 else 'More than 50' - err_message = f"Unable to delete {', '.join(str(obj) for obj in obj_list)}. " \ - f"{protected_count} dependent objects were found: " + if type(e) is ProtectedError: + protected_objects = list(e.protected_objects) + elif type(e) is RestrictedError: + protected_objects = list(e.restricted_objects) + else: + raise e + + # Formulate the error message + err_message = _("Unable to delete {objects}. {count} dependent objects were found: ").format( + objects=', '.join(str(obj) for obj in obj_list), + count=len(protected_objects) if len(protected_objects) <= 50 else _('More than 50') + ) # Append dependent objects to error message dependent_objects = [] diff --git a/netbox/virtualization/api/views.py b/netbox/virtualization/api/views.py index 5b9cf4117..04e8f2167 100644 --- a/netbox/virtualization/api/views.py +++ b/netbox/virtualization/api/views.py @@ -3,6 +3,7 @@ from rest_framework.routers import APIRootView from dcim.models import Device from extras.api.mixins import ConfigContextQuerySetMixin from netbox.api.viewsets import NetBoxModelViewSet +from utilities.query_functions import CollateAsChar from utilities.utils import count_related from virtualization import filtersets from virtualization.models import Cluster, ClusterGroup, ClusterType, VirtualMachine, VMInterface @@ -87,3 +88,7 @@ class VMInterfaceViewSet(NetBoxModelViewSet): serializer_class = serializers.VMInterfaceSerializer filterset_class = filtersets.VMInterfaceFilterSet brief_prefetch_fields = ['virtual_machine'] + + def get_bulk_destroy_queryset(self): + # Ensure child interfaces are deleted prior to their parents + return self.get_queryset().order_by('virtual_machine', 'parent', CollateAsChar('_name')) diff --git a/netbox/virtualization/migrations/0037_protect_child_interfaces.py b/netbox/virtualization/migrations/0037_protect_child_interfaces.py new file mode 100644 index 000000000..ab6cf0cb3 --- /dev/null +++ b/netbox/virtualization/migrations/0037_protect_child_interfaces.py @@ -0,0 +1,19 @@ +# Generated by Django 4.2.6 on 2023-10-20 11:48 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('virtualization', '0036_virtualmachine_config_template'), + ] + + operations = [ + migrations.AlterField( + model_name='vminterface', + name='parent', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.RESTRICT, related_name='child_interfaces', to='virtualization.vminterface'), + ), + ] diff --git a/netbox/virtualization/tests/test_api.py b/netbox/virtualization/tests/test_api.py index b2ae68860..3fb46fbb9 100644 --- a/netbox/virtualization/tests/test_api.py +++ b/netbox/virtualization/tests/test_api.py @@ -293,3 +293,29 @@ class VMInterfaceTest(APIViewTestCases.APIViewTestCase): 'vrf': vrfs[2].pk, }, ] + + def test_bulk_delete_child_interfaces(self): + interface1 = VMInterface.objects.get(name='Interface 1') + virtual_machine = interface1.virtual_machine + self.add_permissions('virtualization.delete_vminterface') + + # Create a child interface + child = VMInterface.objects.create( + virtual_machine=virtual_machine, + name='Interface 1A', + parent=interface1 + ) + self.assertEqual(virtual_machine.interfaces.count(), 4) + + # Attempt to delete only the parent interface + url = self._get_detail_url(interface1) + self.client.delete(url, **self.header) + self.assertEqual(virtual_machine.interfaces.count(), 4) # Parent was not deleted + + # Attempt to bulk delete parent & child together + data = [ + {"id": interface1.pk}, + {"id": child.pk}, + ] + self.client.delete(self._get_list_url(), data, format='json', **self.header) + self.assertEqual(virtual_machine.interfaces.count(), 2) # Child & parent were both deleted diff --git a/netbox/virtualization/tests/test_views.py b/netbox/virtualization/tests/test_views.py index a5d831d7e..f47c386e9 100644 --- a/netbox/virtualization/tests/test_views.py +++ b/netbox/virtualization/tests/test_views.py @@ -374,3 +374,32 @@ class VMInterfaceTestCase(ViewTestCases.DeviceComponentViewTestCase): 'untagged_vlan': vlans[0].pk, 'tagged_vlans': [v.pk for v in vlans[1:4]], } + + def test_bulk_delete_child_interfaces(self): + interface1 = VMInterface.objects.get(name='Interface 1') + virtual_machine = interface1.virtual_machine + self.add_permissions('virtualization.delete_vminterface') + + # Create a child interface + child = VMInterface.objects.create( + virtual_machine=virtual_machine, + name='Interface 1A', + parent=interface1 + ) + self.assertEqual(virtual_machine.interfaces.count(), 4) + + # Attempt to delete only the parent interface + data = { + 'confirm': True, + } + self.client.post(self._get_url('delete', interface1), data) + self.assertEqual(virtual_machine.interfaces.count(), 4) # Parent was not deleted + + # Attempt to bulk delete parent & child together + data = { + 'pk': [interface1.pk, child.pk], + 'confirm': True, + '_confirm': True, # Form button + } + self.client.post(self._get_url('bulk_delete'), data) + self.assertEqual(virtual_machine.interfaces.count(), 2) # Child & parent were both deleted diff --git a/netbox/virtualization/views.py b/netbox/virtualization/views.py index 798d1fc4d..e8782243f 100644 --- a/netbox/virtualization/views.py +++ b/netbox/virtualization/views.py @@ -1,5 +1,4 @@ import traceback -from collections import defaultdict from django.contrib import messages from django.db import transaction @@ -19,6 +18,7 @@ from ipam.tables import InterfaceVLANTable from netbox.constants import DEFAULT_ACTION_PERMISSIONS from netbox.views import generic from tenancy.views import ObjectContactsView +from utilities.query_functions import CollateAsChar from utilities.utils import count_related from utilities.views import ViewTab, register_model_view from . import filtersets, forms, tables @@ -550,7 +550,8 @@ class VMInterfaceBulkRenameView(generic.BulkRenameView): class VMInterfaceBulkDeleteView(generic.BulkDeleteView): - queryset = VMInterface.objects.all() + # Ensure child interfaces are deleted prior to their parents + queryset = VMInterface.objects.order_by('virtual_machine', 'parent', CollateAsChar('_name')) filterset = filtersets.VMInterfaceFilterSet table = tables.VMInterfaceTable From f6338abf14ae1e43e213b13df0293931cb2543c0 Mon Sep 17 00:00:00 2001 From: "Jamie (Bear) Murphy" <1613241+ITJamie@users.noreply.github.com> Date: Wed, 1 Nov 2023 19:13:45 +0000 Subject: [PATCH 5/6] Closes #13690: List all objects to be deleted (#14089) * show objects that would be deleted by cascade * some items were not showing (eg ips on devices) * dont include the item being deleted in the list of related items * Revert "dont include the item being deleted in the list of related items" This reverts commit 298a7860b20c2fd90e887c66c4f196460097e71e. * cleanup - migrate code to use collector directly instead of the NestedObjects wrapper from admin.utils - adjust object names and text output * requested adjustments * remove comma from end of list * linting * refactor, add accordion * migrate to defaultdict, use title for capitalisation of accordian titles * Misc cleanup --------- Co-authored-by: Jeremy Stretch --- netbox/netbox/views/generic/object_views.py | 28 ++++++++++++++++- netbox/templates/htmx/delete_form.html | 34 +++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 7c737aaf0..99508c9e3 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -1,9 +1,11 @@ import logging +from collections import defaultdict from copy import deepcopy from django.contrib import messages -from django.db import transaction +from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError +from django.db.models.deletion import Collector from django.shortcuts import redirect, render from django.urls import reverse from django.utils.html import escape @@ -320,6 +322,27 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): def get_required_permission(self): return get_permission_for_model(self.queryset.model, 'delete') + 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. + + Args: + obj: The object to return dependent objects for + """ + using = router.db_for_write(obj._meta.model) + collector = Collector(using=using) + collector.collect([obj]) + + # Compile a mapping of models to instances + dependent_objects = defaultdict(list) + for model, instance in collector.instances_with_model(): + # Omit the root object + if instance != obj: + dependent_objects[model].append(instance) + + return dict(dependent_objects) + # # Request handlers # @@ -333,6 +356,7 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): """ obj = self.get_object(**kwargs) form = ConfirmationForm(initial=request.GET) + dependent_objects = self._get_dependent_objects(obj) # If this is an HTMX request, return only the rendered deletion form as modal content if is_htmx(request): @@ -343,6 +367,7 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): 'object_type': self.queryset.model._meta.verbose_name, 'form': form, 'form_url': form_url, + 'dependent_objects': dependent_objects, **self.get_extra_context(request, obj), }) @@ -350,6 +375,7 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): 'object': obj, 'form': form, 'return_url': self.get_return_url(request, obj), + 'dependent_objects': dependent_objects, **self.get_extra_context(request, obj), }) diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html index 15f08ebfd..80aec2c82 100644 --- a/netbox/templates/htmx/delete_form.html +++ b/netbox/templates/htmx/delete_form.html @@ -12,6 +12,40 @@ Are you sure you want to delete {{ object_type }} {{ object }}? {% endblocktrans %}

+ {% if dependent_objects %} +

+ {% trans "The following objects will be deleted as a result of this action." %} +

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

+ +

+
+
+
+ {% for instance in instances %} + {% with url=instance.get_absolute_url %} + {{ instance }} + {% endwith %} + {% endfor %} +
+
+
+
+ {% endfor %} +
+ {% endif %} {% render_form form %}
From 8dcbd66de68fab81d074d222e1aafbf54e2d2ba5 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 3 Nov 2023 10:32:59 -0400 Subject: [PATCH 6/6] Closes #13427: Public model registration (#14152) * Initial work on #13427 * Clarify documentation * Reference public models registry when populating models for ConfigTemplate context --- docs/development/application-registry.md | 4 +++ netbox/account/models.py | 2 ++ .../core/migrations/0008_contenttype_proxy.py | 29 +++++++++++++++++ netbox/core/models/__init__.py | 1 + netbox/core/models/contenttypes.py | 32 +++++++++++++++++++ netbox/core/models/data.py | 2 ++ netbox/core/models/files.py | 1 + netbox/dcim/models/cables.py | 2 ++ netbox/extras/dashboard/widgets.py | 9 ++---- netbox/extras/forms/bulk_import.py | 5 ++- netbox/extras/forms/filtersets.py | 5 ++- netbox/extras/forms/model_forms.py | 7 ++-- netbox/extras/models/configs.py | 10 +++--- netbox/extras/models/search.py | 2 ++ netbox/extras/models/tags.py | 2 ++ netbox/extras/utils.py | 4 +++ netbox/netbox/registry.py | 1 + netbox/users/models.py | 2 ++ 18 files changed, 98 insertions(+), 22 deletions(-) create mode 100644 netbox/core/migrations/0008_contenttype_proxy.py create mode 100644 netbox/core/models/contenttypes.py diff --git a/docs/development/application-registry.md b/docs/development/application-registry.md index 41bf6cb31..c7ac6ca46 100644 --- a/docs/development/application-registry.md +++ b/docs/development/application-registry.md @@ -41,6 +41,10 @@ A dictionary of particular features (e.g. custom fields) mapped to the NetBox mo Supported model features are listed in the [features matrix](./models.md#features-matrix). +### `models` + +This key lists all models which have been registered in NetBox which are not designated for private use. (Setting `_netbox_private` to True on a model excludes it from this list.) As with individual features under `model_features`, models are organized by app label. + ### `plugins` This store maintains all registered items for plugins, such as navigation menus, template extensions, etc. diff --git a/netbox/account/models.py b/netbox/account/models.py index 5d6575040..bd5879a85 100644 --- a/netbox/account/models.py +++ b/netbox/account/models.py @@ -7,6 +7,8 @@ class UserToken(Token): """ Proxy model for users to manage their own API tokens. """ + _netbox_private = True + class Meta: proxy = True verbose_name = 'token' diff --git a/netbox/core/migrations/0008_contenttype_proxy.py b/netbox/core/migrations/0008_contenttype_proxy.py new file mode 100644 index 000000000..ac11d906a --- /dev/null +++ b/netbox/core/migrations/0008_contenttype_proxy.py @@ -0,0 +1,29 @@ +# Generated by Django 4.2.6 on 2023-10-31 19:38 + +import core.models.contenttypes +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('core', '0007_job_add_error_field'), + ] + + operations = [ + migrations.CreateModel( + name='ContentType', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('contenttypes.contenttype',), + managers=[ + ('objects', core.models.contenttypes.ContentTypeManager()), + ], + ), + ] diff --git a/netbox/core/models/__init__.py b/netbox/core/models/__init__.py index 185622f5f..c93c392d7 100644 --- a/netbox/core/models/__init__.py +++ b/netbox/core/models/__init__.py @@ -1,3 +1,4 @@ +from .contenttypes import * from .data import * from .files import * from .jobs import * diff --git a/netbox/core/models/contenttypes.py b/netbox/core/models/contenttypes.py new file mode 100644 index 000000000..18c16a1c2 --- /dev/null +++ b/netbox/core/models/contenttypes.py @@ -0,0 +1,32 @@ +from django.contrib.contenttypes.models import ContentType as ContentType_, ContentTypeManager as ContentTypeManager_ +from django.db.models import Q + +from netbox.registry import registry + +__all__ = ( + 'ContentType', + 'ContentTypeManager', +) + + +class ContentTypeManager(ContentTypeManager_): + + def public(self): + """ + Filter the base queryset to return only ContentTypes corresponding to "public" models; those which are listed + in registry['models'] and intended for reference by other objects. + """ + q = Q() + for app_label, models in registry['models'].items(): + q |= Q(app_label=app_label, model__in=models) + return self.get_queryset().filter(q) + + +class ContentType(ContentType_): + """ + Wrap Django's native ContentType model to use our custom manager. + """ + objects = ContentTypeManager() + + class Meta: + proxy = True diff --git a/netbox/core/models/data.py b/netbox/core/models/data.py index fb764134a..78f05e462 100644 --- a/netbox/core/models/data.py +++ b/netbox/core/models/data.py @@ -378,6 +378,8 @@ class AutoSyncRecord(models.Model): fk_field='object_id' ) + _netbox_private = True + class Meta: constraints = ( models.UniqueConstraint( diff --git a/netbox/core/models/files.py b/netbox/core/models/files.py index 38d82463e..138527581 100644 --- a/netbox/core/models/files.py +++ b/netbox/core/models/files.py @@ -44,6 +44,7 @@ class ManagedFile(SyncedDataMixin, models.Model): ) objects = RestrictedQuerySet.as_manager() + _netbox_private = True class Meta: ordering = ('file_root', 'file_path') diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 751bca271..d248b5cad 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -431,6 +431,8 @@ class CablePath(models.Model): ) _nodes = PathField() + _netbox_private = True + class Meta: verbose_name = _('cable path') verbose_name_plural = _('cable paths') diff --git a/netbox/extras/dashboard/widgets.py b/netbox/extras/dashboard/widgets.py index 0b185d432..7f0229f88 100644 --- a/netbox/extras/dashboard/widgets.py +++ b/netbox/extras/dashboard/widgets.py @@ -7,15 +7,13 @@ import feedparser import requests from django import forms from django.conf import settings -from django.contrib.contenttypes.models import ContentType from django.core.cache import cache -from django.db.models import Q from django.template.loader import render_to_string from django.urls import NoReverseMatch, resolve, reverse from django.utils.translation import gettext as _ +from core.models import ContentType from extras.choices import BookmarkOrderingChoices -from extras.utils import FeatureQuery from utilities.choices import ButtonColorChoices from utilities.forms import BootstrapMixin from utilities.permissions import get_permission_for_model @@ -37,10 +35,7 @@ __all__ = ( def get_content_type_labels(): return [ (content_type_identifier(ct), content_type_name(ct)) - for ct in ContentType.objects.filter( - FeatureQuery('export_templates').get_query() | Q(app_label='extras', model='objectchange') | - Q(app_label='extras', model='configcontext') - ).order_by('app_label', 'model') + for ct in ContentType.objects.public().order_by('app_label', 'model') ] diff --git a/netbox/extras/forms/bulk_import.py b/netbox/extras/forms/bulk_import.py index 79023a74d..03a6d118b 100644 --- a/netbox/extras/forms/bulk_import.py +++ b/netbox/extras/forms/bulk_import.py @@ -1,9 +1,9 @@ from django import forms -from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.forms import SimpleArrayField from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ +from core.models import ContentType from extras.choices import * from extras.models import * from extras.utils import FeatureQuery @@ -40,8 +40,7 @@ class CustomFieldImportForm(CSVModelForm): ) object_type = CSVContentTypeField( label=_('Object type'), - queryset=ContentType.objects.all(), - limit_choices_to=FeatureQuery('custom_fields'), + queryset=ContentType.objects.public(), required=False, help_text=_("Object type (for object or multi-object fields)") ) diff --git a/netbox/extras/forms/filtersets.py b/netbox/extras/forms/filtersets.py index 7db84d175..c0c8835b4 100644 --- a/netbox/extras/forms/filtersets.py +++ b/netbox/extras/forms/filtersets.py @@ -1,9 +1,8 @@ from django import forms from django.contrib.auth import get_user_model -from django.contrib.contenttypes.models import ContentType from django.utils.translation import gettext_lazy as _ -from core.models import DataFile, DataSource +from core.models import ContentType, DataFile, DataSource from dcim.models import DeviceRole, DeviceType, Location, Platform, Region, Site, SiteGroup from extras.choices import * from extras.models import * @@ -196,7 +195,7 @@ class SavedFilterFilterForm(SavedFiltersMixin, FilterForm): ) content_types = ContentTypeMultipleChoiceField( label=_('Content types'), - queryset=ContentType.objects.filter(FeatureQuery('export_templates').get_query()), + queryset=ContentType.objects.public(), required=False ) enabled = forms.NullBooleanField( diff --git a/netbox/extras/forms/model_forms.py b/netbox/extras/forms/model_forms.py index fd2ce8f2d..7ab568ae0 100644 --- a/netbox/extras/forms/model_forms.py +++ b/netbox/extras/forms/model_forms.py @@ -2,12 +2,11 @@ import json from django import forms from django.conf import settings -from django.db.models import Q -from django.contrib.contenttypes.models import ContentType from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ from core.forms.mixins import SyncedDataMixin +from core.models import ContentType from dcim.models import DeviceRole, DeviceType, Location, Platform, Region, Site, SiteGroup from extras.choices import * from extras.models import * @@ -49,9 +48,7 @@ class CustomFieldForm(BootstrapMixin, forms.ModelForm): ) object_type = ContentTypeChoiceField( label=_('Object type'), - queryset=ContentType.objects.all(), - # TODO: Come up with a canonical way to register suitable models - limit_choices_to=FeatureQuery('webhooks').get_query() | Q(app_label='auth', model__in=['user', 'group']), + queryset=ContentType.objects.public(), required=False, help_text=_("Type of the related object (for object/multi-object fields only)") ) diff --git a/netbox/extras/models/configs.py b/netbox/extras/models/configs.py index 2acfcb725..425c1386a 100644 --- a/netbox/extras/models/configs.py +++ b/netbox/extras/models/configs.py @@ -260,12 +260,14 @@ class ConfigTemplate(SyncedDataMixin, ExportTemplatesMixin, TagsMixin, ChangeLog _context = dict() # Populate the default template context with NetBox model classes, namespaced by app - # TODO: Devise a canonical mechanism for identifying the models to include (see #13427) - for app, model_names in registry['model_features']['custom_fields'].items(): + for app, model_names in registry['models'].items(): _context.setdefault(app, {}) for model_name in model_names: - model = apps.get_registered_model(app, model_name) - _context[app][model.__name__] = model + try: + model = apps.get_registered_model(app, model_name) + _context[app][model.__name__] = model + except LookupError: + pass # Add the provided context data, if any if context is not None: diff --git a/netbox/extras/models/search.py b/netbox/extras/models/search.py index 39ff80215..b3327d510 100644 --- a/netbox/extras/models/search.py +++ b/netbox/extras/models/search.py @@ -49,6 +49,8 @@ class CachedValue(models.Model): default=1000 ) + _netbox_private = True + class Meta: ordering = ('weight', 'object_type', 'value', 'object_id') verbose_name = _('cached value') diff --git a/netbox/extras/models/tags.py b/netbox/extras/models/tags.py index f4ba5ea64..de4f15509 100644 --- a/netbox/extras/models/tags.py +++ b/netbox/extras/models/tags.py @@ -75,6 +75,8 @@ class TaggedItem(GenericTaggedItemBase): on_delete=models.CASCADE ) + _netbox_private = True + class Meta: indexes = [models.Index(fields=["content_type", "object_id"])] verbose_name = _('tagged item') diff --git a/netbox/extras/utils.py b/netbox/extras/utils.py index 23892e098..7b9356efb 100644 --- a/netbox/extras/utils.py +++ b/netbox/extras/utils.py @@ -67,6 +67,10 @@ def register_features(model, features): f"{feature} is not a valid model feature! Valid keys are: {registry['model_features'].keys()}" ) + # Register public models + if not getattr(model, '_netbox_private', False): + registry['models'][app_label].add(model_name) + def is_script(obj): """ diff --git a/netbox/netbox/registry.py b/netbox/netbox/registry.py index 21a869001..151eb2f6b 100644 --- a/netbox/netbox/registry.py +++ b/netbox/netbox/registry.py @@ -25,6 +25,7 @@ registry = Registry({ 'data_backends': dict(), 'denormalized_fields': collections.defaultdict(list), 'model_features': dict(), + 'models': collections.defaultdict(set), 'plugins': dict(), 'search': dict(), 'views': collections.defaultdict(dict), diff --git a/netbox/users/models.py b/netbox/users/models.py index 1f8772704..2a345653d 100644 --- a/netbox/users/models.py +++ b/netbox/users/models.py @@ -99,6 +99,8 @@ class UserConfig(models.Model): default=dict ) + _netbox_private = True + class Meta: ordering = ['user'] verbose_name = _('user preferences')