diff --git a/netbox/dcim/forms/bulk_import.py b/netbox/dcim/forms/bulk_import.py index 6f3197919..f756672fe 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -1583,7 +1583,8 @@ class CableImportForm(PrimaryModelImportForm): :param side: 'a' or 'b' """ - assert side in 'ab', f"Invalid side designation: {side}" + if side not in ('a', 'b'): + raise ValueError(_("Invalid side designation: {side}").format(side=side)) device = self.cleaned_data.get(f'side_{side}_device') power_panel = self.cleaned_data.get(f'side_{side}_power_panel') diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 218300a53..d1c70d942 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -675,7 +675,10 @@ class CableTermination(ChangeLoggedModel): Cache objects related to the termination (e.g. device, rack, site) directly on the object to enable efficient filtering. """ - assert self.termination is not None + if self.termination is None: + raise ValueError( + _("Invalid cable termination: the assigned termination object does not exist.") + ) # Device components if getattr(self.termination, 'device', None): diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 3fe568384..2bb0d241d 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -503,6 +503,15 @@ class InterfaceTestCase(TestCase): self.assertNotIn('qinq_svlan', form.cleaned_data.keys()) +class CableTestCase(TestCase): + + def test_invalid_side_designation_raises_value_error(self): + """_clean_side rejects a side other than 'a' or 'b' with ValueError.""" + form = CableImportForm.__new__(CableImportForm) + with self.assertRaisesMessage(ValueError, "Invalid side designation: c"): + form._clean_side('c') + + class SiteFormTestCase(TestCase): """ Tests for M2MAddRemoveFields using Site ASN assignments as the test case. diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 8c2c6a2d2..dc3c2bd24 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -2189,6 +2189,18 @@ class CableTestCase(TestCase): self.assertIsNone(interface.path) +class CableTerminationTestCase(TestCase): + + def test_cache_related_objects_requires_resolvable_termination(self): + """cache_related_objects raises ValueError when the termination cannot be resolved.""" + cable_termination = CableTermination( + termination_type=ObjectType.objects.get_for_model(Interface), + termination_id=0, + ) + with self.assertRaises(ValueError): + cable_termination.cache_related_objects() + + class VirtualDeviceContextTestCase(TestCase): @classmethod diff --git a/netbox/extras/events.py b/netbox/extras/events.py index 470adc875..a2f3aac78 100644 --- a/netbox/extras/events.py +++ b/netbox/extras/events.py @@ -125,7 +125,13 @@ def enqueue_event(queue, instance, request, event_type): app_label = instance._meta.app_label model_name = instance._meta.model_name - assert instance.pk is not None + if instance.pk is None: + raise ValueError( + _("Cannot enqueue an event for an unsaved {app_label}.{model} instance.").format( + app_label=app_label, + model=model_name, + ) + ) key = f'{app_label}.{model_name}:{instance.pk}' if key in queue: diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 8bf21d175..42faf5bdc 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -39,6 +39,16 @@ class EventRuleTestCase(APITestCase): # Clear the queue so leftover jobs do not leak to the next test suite self.queue.empty() + def test_enqueue_event_requires_saved_instance(self): + """enqueue_event raises ValueError for an unsaved instance.""" + request = RequestFactory().get('/') + request.id = uuid.uuid4() + request.user = self.user + site = Site(name='Site 1', slug='site-1') + with patch('extras.events.has_feature', return_value=True): + with self.assertRaises(ValueError): + enqueue_event({}, site, request, OBJECT_CREATED) + @classmethod def setUpTestData(cls): diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index e106fbc10..86ac2bfd7 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -3,6 +3,7 @@ import logging from django.contrib.contenttypes.fields import GenericRelation from django.db import router from django.db.models.deletion import CASCADE, Collector +from django.utils.translation import gettext as _ logger = logging.getLogger("netbox.models.deletion") @@ -45,7 +46,7 @@ class CustomCollector(Collector): # Add GenericRelations to the dependency graph processed_relations = set() - for _, instances in list(self.data.items()): + for _model, instances in list(self.data.items()): for instance in instances: # Get all GenericRelations for this model for field in instance._meta.private_fields: @@ -70,10 +71,13 @@ class DeleteMixin: Override delete to use our custom collector. """ using = using or router.db_for_write(self.__class__, instance=self) - assert self._get_pk_val() is not None, ( - f"{self._meta.object_name} object can't be deleted because its " - f"{self._meta.pk.attname} attribute is set to None." - ) + if self._get_pk_val() is None: + raise ValueError( + _("{object_name} object can't be deleted because its {pk_attname} attribute is set to None.").format( + object_name=self._meta.object_name, + pk_attname=self._meta.pk.attname, + ) + ) collector = CustomCollector(using=using) collector.collect([self], keep_parents=keep_parents) diff --git a/netbox/netbox/tests/test_models.py b/netbox/netbox/tests/test_models.py index 0351aea82..d4a931a0d 100644 --- a/netbox/netbox/tests/test_models.py +++ b/netbox/netbox/tests/test_models.py @@ -4,6 +4,7 @@ from django.conf import settings from django.test import TestCase from core.models import ObjectChange +from dcim.models import Site from netbox.tests.dummy_plugin.models import DummyNetBoxModel @@ -21,3 +22,12 @@ class ModelTestCase(TestCase): m.pk = 123 self.assertEqual(m.get_absolute_url(), f'/plugins/dummy-plugin/netboxmodel/{m.pk}/') + + +class DeleteMixinTestCase(TestCase): + + def test_delete_unsaved_instance_raises_value_error(self): + """Deleting an instance with no primary key raises ValueError.""" + site = Site(name='Site 1', slug='site-1') + with self.assertRaises(ValueError): + site.delete() diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index bdd7528d0..65e2a58ae 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -7,7 +7,7 @@ from types import SimpleNamespace from django.conf import settings from django.contrib import messages from django.contrib.contenttypes.fields import GenericForeignKey, GenericRel -from django.core.exceptions import FieldDoesNotExist, ObjectDoesNotExist, ValidationError +from django.core.exceptions import FieldDoesNotExist, ImproperlyConfigured, ObjectDoesNotExist, ValidationError from django.db import IntegrityError, router, transaction from django.db.models import ManyToManyField, ProtectedError, RestrictedError from django.db.models.fields.reverse_related import ManyToManyRel @@ -731,7 +731,10 @@ class BulkEditView(GetReturnURLMixin, BaseMultiObjectView): # Update custom fields for name, customfield in custom_fields.items(): - assert name.startswith('cf_') + if not name.startswith('cf_'): + raise ImproperlyConfigured( + _("Custom field form field name must begin with 'cf_': {name}").format(name=name) + ) cf_name = name[3:] # Strip cf_ prefix if name in form.nullable_fields and name in nullified_fields: obj.custom_field_data[cf_name] = None diff --git a/netbox/utilities/choices.py b/netbox/utilities/choices.py index 80151e807..60b24f0df 100644 --- a/netbox/utilities/choices.py +++ b/netbox/utilities/choices.py @@ -1,6 +1,7 @@ import enum from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.utils.translation import gettext_lazy as _ from utilities.data import get_config_value_ci @@ -20,9 +21,10 @@ class ChoiceSetMeta(type): # Extend static choices with any configured choices if key := attrs.get('key'): - assert type(attrs['CHOICES']) is list, _( - "{name} has a key defined but CHOICES is not a list" - ).format(name=name) + if type(attrs['CHOICES']) is not list: + raise ImproperlyConfigured( + _("{name} has a key defined but CHOICES is not a list").format(name=name) + ) app = attrs['__module__'].split('.', 1)[0] replace_key = f'{app}.{key}' replace_choices = get_config_value_ci(settings.FIELD_CHOICES, replace_key) diff --git a/netbox/utilities/tests/test_choices.py b/netbox/utilities/tests/test_choices.py index 2bfd8fd5d..4005dfcec 100644 --- a/netbox/utilities/tests/test_choices.py +++ b/netbox/utilities/tests/test_choices.py @@ -1,3 +1,4 @@ +from django.core.exceptions import ImproperlyConfigured from django.test import TestCase, override_settings from utilities.choices import ChoiceSet @@ -31,6 +32,19 @@ class ChoiceSetTestCase(TestCase): def test_values(self): self.assertListEqual(ExampleChoices.values(), ['a', 'b', 'c', 1, 2, 3]) + def test_key_with_non_list_choices_raises(self): + """A ChoiceSet declaring a key must define CHOICES as a list.""" + with self.assertRaises(ImproperlyConfigured): + type( + 'InvalidChoices', + (ChoiceSet,), + { + '__module__': __name__, + 'key': 'invalid_choices', + 'CHOICES': (('foo', 'Foo'),), + }, + ) + class FieldChoicesCaseInsensitiveTestCase(TestCase): """