From 47f3994f933ddede4521a4981adbf92e72b4482d Mon Sep 17 00:00:00 2001 From: Daniel Sheppard Date: Sun, 2 Jun 2024 22:06:34 -0500 Subject: [PATCH 1/4] Catch AssertionError from cable trace and throw ValidationError --- netbox/dcim/models/cables.py | 21 +++++++++++++-------- netbox/netbox/views/generic/object_views.py | 7 +++++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 64f0b85606d..e9b58e413af 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -237,7 +237,10 @@ def save(self, *args, **kwargs): if not termination.pk or termination not in b_terminations: CableTermination(cable=self, cable_end='B', termination=termination).save() - trace_paths.send(Cable, instance=self, created=_created) + try: + trace_paths.send(Cable, instance=self, created=_created) + except ValidationError as e: + raise ValidationError(f'{e}') def get_status_color(self): return LinkStatusChoices.colors.get(self.status) @@ -532,7 +535,8 @@ def from_origin(cls, terminations): # Ensure all originating terminations are attached to the same link if len(terminations) > 1: - assert all(t.link == terminations[0].link for t in terminations[1:]) + assert all(t.link == terminations[0].link for t in terminations[1:]), \ + "All originating terminations must start must be attached to the same link" path = [] position_stack = [] @@ -543,12 +547,13 @@ def from_origin(cls, terminations): while terminations: # Terminations must all be of the same type - assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]) + assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]), \ + "All mid-span terminations must have the same termination type" # All mid-span terminations must all be attached to the same device if not isinstance(terminations[0], PathEndpoint): - assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]) - assert all(t.parent_object == terminations[0].parent_object for t in terminations[1:]) + assert all(t.parent_object == terminations[0].parent_object for t in terminations[1:]), \ + "All mid-span terminations must have the same parent object" # Check for a split path (e.g. rear port fanning out to multiple front ports with # different cables attached) @@ -571,8 +576,8 @@ def from_origin(cls, terminations): return None # Otherwise, halt the trace if no link exists break - assert all(type(link) in (Cable, WirelessLink) for link in links) - assert all(isinstance(link, type(links[0])) for link in links) + assert all(type(link) in (Cable, WirelessLink) for link in links), "All links must be cable or wireless" + assert all(isinstance(link, type(links[0])) for link in links), "All links must match first link type" # Step 3: Record asymmetric paths as split not_connected_terminations = [termination.link for termination in terminations if termination.link is None] @@ -656,7 +661,7 @@ def from_origin(cls, terminations): for rt in remote_terminations: position = positions.pop() q_filter |= Q(rear_port_id=rt.pk, rear_port_position=position) - assert q_filter is not Q() + assert q_filter is not Q(), "Remote termination query filter is empty, please open a bug report" front_ports = FrontPort.objects.filter(q_filter) # Obtain the individual front ports based on the termination and position elif position_stack: diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 243ae2547d8..5335a974e0d 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -3,6 +3,7 @@ from copy import deepcopy from django.contrib import messages +from django.core.exceptions import ValidationError from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError from django.db.models.deletion import Collector @@ -307,6 +308,12 @@ def post(self, request, *args, **kwargs): form.add_error(None, e.message) clear_events.send(sender=self) + # Catch any validation errors thrown in the model.save() or form.save() methods + except ValidationError as e: + logger.debug(e.message) + form.add_error(None, e.message) + clear_events.send(sender=self) + else: logger.debug("Form validation failed") From 7c913fecb7cec53ef84fc19796b759dca4c18ef4 Mon Sep 17 00:00:00 2001 From: Daniel Sheppard Date: Sun, 2 Jun 2024 22:13:23 -0500 Subject: [PATCH 2/4] Change to translation strings --- netbox/dcim/models/cables.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index e9b58e413af..86a610e2428 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -535,8 +535,9 @@ def from_origin(cls, terminations): # Ensure all originating terminations are attached to the same link if len(terminations) > 1: - assert all(t.link == terminations[0].link for t in terminations[1:]), \ - "All originating terminations must start must be attached to the same link" + assert all(t.link == terminations[0].link for t in terminations[1:]), ( + _("All originating terminations must start must be attached to the same link") + ) path = [] position_stack = [] @@ -547,13 +548,15 @@ def from_origin(cls, terminations): while terminations: # Terminations must all be of the same type - assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]), \ - "All mid-span terminations must have the same termination type" + assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]), ( + _("All mid-span terminations must have the same termination type") + ) # All mid-span terminations must all be attached to the same device if not isinstance(terminations[0], PathEndpoint): - assert all(t.parent_object == terminations[0].parent_object for t in terminations[1:]), \ - "All mid-span terminations must have the same parent object" + assert all(t.parent_object == terminations[0].parent_object for t in terminations[1:]), ( + _("All mid-span terminations must have the same parent object") + ) # Check for a split path (e.g. rear port fanning out to multiple front ports with # different cables attached) @@ -576,8 +579,8 @@ def from_origin(cls, terminations): return None # Otherwise, halt the trace if no link exists break - assert all(type(link) in (Cable, WirelessLink) for link in links), "All links must be cable or wireless" - assert all(isinstance(link, type(links[0])) for link in links), "All links must match first link type" + assert all(type(link) in (Cable, WirelessLink) for link in links), _("All links must be cable or wireless") + assert all(isinstance(link, type(links[0])) for link in links), _("All links must match first link type") # Step 3: Record asymmetric paths as split not_connected_terminations = [termination.link for termination in terminations if termination.link is None] @@ -654,14 +657,16 @@ def from_origin(cls, terminations): positions = position_stack.pop() # Ensure we have a number of positions equal to the amount of remote terminations - assert len(remote_terminations) == len(positions) + assert len(remote_terminations) == len(positions), ( + _("All positions counts within the path on opposite ends of links must match") + ) # Get our front ports q_filter = Q() for rt in remote_terminations: position = positions.pop() q_filter |= Q(rear_port_id=rt.pk, rear_port_position=position) - assert q_filter is not Q(), "Remote termination query filter is empty, please open a bug report" + assert q_filter is not Q(), _("Remote termination query filter is empty, please open a bug report") front_ports = FrontPort.objects.filter(q_filter) # Obtain the individual front ports based on the termination and position elif position_stack: From 7f486517cc947847987ef43340acf70b8739cac2 Mon Sep 17 00:00:00 2001 From: Daniel Sheppard Date: Fri, 7 Jun 2024 08:40:07 -0500 Subject: [PATCH 3/4] Convert from assertions to conditionals and raise exception directly. --- netbox/dcim/models/cables.py | 39 +++++++++++++--------------- netbox/dcim/tests/test_cablepaths.py | 5 ++-- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 86a610e2428..032d40cec4f 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -237,10 +237,7 @@ def save(self, *args, **kwargs): if not termination.pk or termination not in b_terminations: CableTermination(cable=self, cable_end='B', termination=termination).save() - try: - trace_paths.send(Cable, instance=self, created=_created) - except ValidationError as e: - raise ValidationError(f'{e}') + trace_paths.send(Cable, instance=self, created=_created) def get_status_color(self): return LinkStatusChoices.colors.get(self.status) @@ -534,10 +531,8 @@ def from_origin(cls, terminations): return None # Ensure all originating terminations are attached to the same link - if len(terminations) > 1: - assert all(t.link == terminations[0].link for t in terminations[1:]), ( - _("All originating terminations must start must be attached to the same link") - ) + if len(terminations) > 1 and not all(t.link == terminations[0].link for t in terminations[1:]): + raise ValidationError(_("All originating terminations must start must be attached to the same link")) path = [] position_stack = [] @@ -548,15 +543,13 @@ def from_origin(cls, terminations): while terminations: # Terminations must all be of the same type - assert all(isinstance(t, type(terminations[0])) for t in terminations[1:]), ( - _("All mid-span terminations must have the same termination type") - ) + if not all(isinstance(t, type(terminations[0])) for t in terminations[1:]): + raise ValidationError(_("All mid-span terminations must have the same termination type")) # All mid-span terminations must all be attached to the same device - if not isinstance(terminations[0], PathEndpoint): - assert all(t.parent_object == terminations[0].parent_object for t in terminations[1:]), ( - _("All mid-span terminations must have the same parent object") - ) + if (not isinstance(terminations[0], PathEndpoint) and not + all(t.parent_object == terminations[0].parent_object for t in terminations[1:])): + raise ValidationError(_("All mid-span terminations must have the same parent object")) # Check for a split path (e.g. rear port fanning out to multiple front ports with # different cables attached) @@ -579,8 +572,10 @@ def from_origin(cls, terminations): return None # Otherwise, halt the trace if no link exists break - assert all(type(link) in (Cable, WirelessLink) for link in links), _("All links must be cable or wireless") - assert all(isinstance(link, type(links[0])) for link in links), _("All links must match first link type") + if not all(type(link) in (Cable, WirelessLink) for link in links): + raise ValidationError(_("All links must be cable or wireless")) + if not all(isinstance(link, type(links[0])) for link in links): + raise ValidationError(_("All links must match first link type")) # Step 3: Record asymmetric paths as split not_connected_terminations = [termination.link for termination in terminations if termination.link is None] @@ -657,16 +652,18 @@ def from_origin(cls, terminations): positions = position_stack.pop() # Ensure we have a number of positions equal to the amount of remote terminations - assert len(remote_terminations) == len(positions), ( - _("All positions counts within the path on opposite ends of links must match") - ) + if len(remote_terminations) != len(positions): + raise ValidationError( + _("All positions counts within the path on opposite ends of links must match") + ) # Get our front ports q_filter = Q() for rt in remote_terminations: position = positions.pop() q_filter |= Q(rear_port_id=rt.pk, rear_port_position=position) - assert q_filter is not Q(), _("Remote termination query filter is empty, please open a bug report") + if q_filter is Q(): + raise ValidationError(_("Remote termination position filter is missing")) front_ports = FrontPort.objects.filter(q_filter) # Obtain the individual front ports based on the termination and position elif position_stack: diff --git a/netbox/dcim/tests/test_cablepaths.py b/netbox/dcim/tests/test_cablepaths.py index cd7b0e6d79a..38939022198 100644 --- a/netbox/dcim/tests/test_cablepaths.py +++ b/netbox/dcim/tests/test_cablepaths.py @@ -1,3 +1,4 @@ +from django.core.exceptions import ValidationError from django.test import TestCase from circuits.models import * @@ -2261,7 +2262,7 @@ def test_401_exclude_midspan_devices(self): b_terminations=[frontport1, frontport3], label='C1' ) - with self.assertRaises(AssertionError): + with self.assertRaises(ValidationError): cable1.save() self.assertPathDoesNotExist( @@ -2280,7 +2281,7 @@ def test_401_exclude_midspan_devices(self): label='C3' ) - with self.assertRaises(AssertionError): + with self.assertRaises(ValidationError): cable3.save() self.assertPathDoesNotExist( From d4d4e5058b1490cb7f04ea170c621cb0d3b87068 Mon Sep 17 00:00:00 2001 From: Daniel Sheppard Date: Mon, 1 Jul 2024 22:17:50 -0500 Subject: [PATCH 4/4] Update CablePath to throw a "UnsupportedCablePath" exception instead of a validation error. --- netbox/dcim/exceptions.py | 2 ++ netbox/dcim/models/cables.py | 15 ++++++++------- netbox/dcim/tests/test_cablepaths.py | 6 +++--- netbox/netbox/views/generic/object_views.py | 4 ++-- 4 files changed, 15 insertions(+), 12 deletions(-) create mode 100644 netbox/dcim/exceptions.py diff --git a/netbox/dcim/exceptions.py b/netbox/dcim/exceptions.py new file mode 100644 index 00000000000..e4be1b5f1fe --- /dev/null +++ b/netbox/dcim/exceptions.py @@ -0,0 +1,2 @@ +class UnsupportedCablePath(Exception): + pass diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 032d40cec4f..7c7f0ad96af 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -27,6 +27,7 @@ 'CableTermination', ) +from ..exceptions import UnsupportedCablePath trace_paths = Signal() @@ -532,7 +533,7 @@ def from_origin(cls, terminations): # Ensure all originating terminations are attached to the same link if len(terminations) > 1 and not all(t.link == terminations[0].link for t in terminations[1:]): - raise ValidationError(_("All originating terminations must start must be attached to the same link")) + raise UnsupportedCablePath(_("All originating terminations must start must be attached to the same link")) path = [] position_stack = [] @@ -544,12 +545,12 @@ def from_origin(cls, terminations): # Terminations must all be of the same type if not all(isinstance(t, type(terminations[0])) for t in terminations[1:]): - raise ValidationError(_("All mid-span terminations must have the same termination type")) + raise UnsupportedCablePath(_("All mid-span terminations must have the same termination type")) # All mid-span terminations must all be attached to the same device if (not isinstance(terminations[0], PathEndpoint) and not all(t.parent_object == terminations[0].parent_object for t in terminations[1:])): - raise ValidationError(_("All mid-span terminations must have the same parent object")) + raise UnsupportedCablePath(_("All mid-span terminations must have the same parent object")) # Check for a split path (e.g. rear port fanning out to multiple front ports with # different cables attached) @@ -573,9 +574,9 @@ def from_origin(cls, terminations): # Otherwise, halt the trace if no link exists break if not all(type(link) in (Cable, WirelessLink) for link in links): - raise ValidationError(_("All links must be cable or wireless")) + raise UnsupportedCablePath(_("All links must be cable or wireless")) if not all(isinstance(link, type(links[0])) for link in links): - raise ValidationError(_("All links must match first link type")) + raise UnsupportedCablePath(_("All links must match first link type")) # Step 3: Record asymmetric paths as split not_connected_terminations = [termination.link for termination in terminations if termination.link is None] @@ -653,7 +654,7 @@ def from_origin(cls, terminations): # Ensure we have a number of positions equal to the amount of remote terminations if len(remote_terminations) != len(positions): - raise ValidationError( + raise UnsupportedCablePath( _("All positions counts within the path on opposite ends of links must match") ) @@ -663,7 +664,7 @@ def from_origin(cls, terminations): position = positions.pop() q_filter |= Q(rear_port_id=rt.pk, rear_port_position=position) if q_filter is Q(): - raise ValidationError(_("Remote termination position filter is missing")) + raise UnsupportedCablePath(_("Remote termination position filter is missing")) front_ports = FrontPort.objects.filter(q_filter) # Obtain the individual front ports based on the termination and position elif position_stack: diff --git a/netbox/dcim/tests/test_cablepaths.py b/netbox/dcim/tests/test_cablepaths.py index 38939022198..b6161af915d 100644 --- a/netbox/dcim/tests/test_cablepaths.py +++ b/netbox/dcim/tests/test_cablepaths.py @@ -1,8 +1,8 @@ -from django.core.exceptions import ValidationError from django.test import TestCase from circuits.models import * from dcim.choices import LinkStatusChoices +from dcim.exceptions import UnsupportedCablePath from dcim.models import * from dcim.svg import CableTraceSVG from dcim.utils import object_to_path_node @@ -2262,7 +2262,7 @@ def test_401_exclude_midspan_devices(self): b_terminations=[frontport1, frontport3], label='C1' ) - with self.assertRaises(ValidationError): + with self.assertRaises(UnsupportedCablePath): cable1.save() self.assertPathDoesNotExist( @@ -2281,7 +2281,7 @@ def test_401_exclude_midspan_devices(self): label='C3' ) - with self.assertRaises(ValidationError): + with self.assertRaises(UnsupportedCablePath): cable3.save() self.assertPathDoesNotExist( diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 5335a974e0d..f85a515080c 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -3,7 +3,6 @@ from copy import deepcopy from django.contrib import messages -from django.core.exceptions import ValidationError from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError from django.db.models.deletion import Collector @@ -14,6 +13,7 @@ from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ +from dcim.exceptions import UnsupportedCablePath from extras.signals import clear_events from utilities.error_handlers import handle_protectederror from utilities.exceptions import AbortRequest, PermissionsViolation @@ -309,7 +309,7 @@ def post(self, request, *args, **kwargs): clear_events.send(sender=self) # Catch any validation errors thrown in the model.save() or form.save() methods - except ValidationError as e: + except UnsupportedCablePath as e: logger.debug(e.message) form.add_error(None, e.message) clear_events.send(sender=self)