Fixes: #15016 - Catch AssertionError from cable trace and throw ValidationError

[DanSheps]

Fixes: #15016 - Catch AssertionError from cable trace and throw ValidationError

• Catch any assertion errors thrown by the Cable save signal and raise a validation error
• Catch validation error in ObjectEditView and report error in UI while re-rendering form

[jeremystretch]

The use of assert statements in this logic was primarily for sanity-checking during initial development; they shouldn't be relied upon as true validation checks. (They aren't even run when __debug__ is False.)

If the use case in #15016 is not supported, we need to add specific validation logic to check for and report on it.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[jeremystretch]

I'm afraid this still doesn't address my primary concern: Validation of cable state needs to be performed prior to the point where we're generating cable paths. (Again, the original assertions were just sanity checks.) This should ideally be performed on each model under clean().

Additionally, as a rule generic views should not check for and handle any model-specific exceptions.

[DanSheps]

I'm afraid this still doesn't address my primary concern: Validation of cable state needs to be performed prior to the point where we're generating cable paths. (Again, the original assertions were just sanity checks.) This should ideally be performed on each model under clean().

The issue is there isn't really a way to validate the cable state prior to the cable being saved, without effectively duplicating all the work done in the cable path generation.

The main reason for this is because a lot of these assertion checks are dependant on sibling cable state further down the path, not just part of the existing cable save, which isn't known until you generate the full path.

We would effectively be running the from_origin twice in this instance, once to validate the cable state and once to save it.

Take for example this path, where Switch 1, interface1 is what you are connecting (the rest is fully formed):

                     /----[FP1-1]                       [FP2-1]----[FP3-1]                      [FP4-1]---- 
[Switch1][Interface1]             [PP1][RP]----[RP][PP2]                   [PP3][RP]----[RP][PP4]                 
                     \----[FP1-2]                       [FP2-2]----[FP3-2]                      [FP4-2]----

You can validate FP1-1 and FP1-2 because they are connected.

But you can't run any of the peer validations on FP2-1 and FP2-2 because FP2-1 doesn't know that FP2-2 will be part of that same path until Interface1 is connect and that path is fully formed.

I can take another pass at it, and see if I can have it run this in clean() first, but most of the parts of this rely on objects existing in the database already as well, so this would likely need a complete re-write, which I was trying to avoid doing.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[DanSheps]

This is now ready. I stupidly missed an UnsupportedCablePath in the test instead of AbortRequest

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[arthanson]

Dan, can you please rebase to main

[arthanson]

One more @DanSheps when I switch to this branch NetBox won't start with the following error - can you please sync and try:

  File "/Users/ahanson/dev/work/netbox/netbox/netbox/urls.py", line 7, in <module>
    from account.views import LoginView, LogoutView
  File "/Users/ahanson/dev/work/netbox/netbox/account/views.py", line 28, in <module>
    from netbox.views import generic
  File "/Users/ahanson/dev/work/netbox/netbox/netbox/views/generic/__init__.py", line 2, in <module>
    from .feature_views import *
  File "/Users/ahanson/dev/work/netbox/netbox/netbox/views/generic/feature_views.py", line 12, in <module>
    from extras.forms import JournalEntryForm
  File "/Users/ahanson/dev/work/netbox/netbox/extras/forms/__init__.py", line 4, in <module>
    from .bulk_import import *
  File "/Users/ahanson/dev/work/netbox/netbox/extras/forms/bulk_import.py", line 184, in <module>
    class EventRuleImportForm(NetBoxModelImportForm):
  File "/Users/ahanson/dev/work/netbox/venv/lib/python3.12/site-packages/django/forms/models.py", line 312, in __new__
    fields = fields_for_model(
             ^^^^^^^^^^^^^^^^^
  File "/Users/ahanson/dev/work/netbox/venv/lib/python3.12/site-packages/django/forms/models.py", line 207, in fields_for_model
    raise FieldError(
django.core.exceptions.FieldError: 'action_object' cannot be specified for EventRule model form as it is a non-editable field

[DanSheps]

Merged in main. Started clean for me either way but hopefully this corrects whatever you are hitting.

[arthanson]

dcim.model.cables.CablePath.from_origin() is called from dcim.utils.create_cablepath which is called from wireless.signals.update_connected_interfaces in a post_save handler. This would be an issue as it could raise an error post_save so it would already be saved in this incorrect state.

I agree with Jeremy's prior comment that I think we need to validate it before save. Seems like we would want to create the connection graph in memory and then validate that... which sounds like a big pain.

[DanSheps]

I agree with Jeremy's prior comment that I think we need to validate it before save. Seems like we would want to create the connection graph in memory and then validate that... which sounds like a big pain.

Short of re-writing the whole thing, I don't know if we can write it in-memory as-is due to some of the queries that are run against it.

The signals will all fire inside of a transaction which should be rolled back automatically if there is an error. I don't think there is a data consistency concern here, it would be treated the same as if the exception is raise directly in save (the transaction would roll back and you would be presented with the form again).

[arthanson]

You are right on the transaction prevent save, but the raised error won't be caught in this case - to make it easy to test I just put into wireless.signals to force the raise:

@receiver(post_save, sender=WirelessLink)
def update_connected_interfaces(instance, created, raw=False, **kwargs):
    """
    When a WirelessLink is saved, save a reference to it on each connected interface.
    """
    from dcim.exceptions import UnsupportedCablePath
    raise UnsupportedCablePath("All mid-span terminations must have the same parent object")

Then created a wireless link, resulting in

So it's about the same as the assert in this specific case (i.e. no nice validation message popup). Not sure if that is a separate issue.

[DanSheps]

I think catching that is a separate issue and I think we can wrap that in this PR as it is similar.

Let me go add that and see if I can adjust to catch that as well.

[arthanson]

also, do you have an easier repro scenario for the original cable error?

[DanSheps]

also, do you have an easier repro scenario for the original cable error?

Unfortunately no

From message " it's been too long since I looked at it. If you're comfortable dismissing the old review, it's fine with me"