Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

DanSheps
Copy link
Member

@DanSheps DanSheps commented Jun 3, 2024

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

@DanSheps DanSheps self-assigned this Jun 3, 2024
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

netbox/dcim/models/cables.py Outdated Show resolved Hide resolved
netbox/dcim/models/cables.py Outdated Show resolved Hide resolved
Copy link
Contributor

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 github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jun 26, 2024
@DanSheps DanSheps removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 2, 2024
Copy link
Contributor

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 github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 17, 2024
@DanSheps DanSheps removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 22, 2024
Copy link
Contributor

github-actions bot commented Aug 7, 2024

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 github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 7, 2024
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 16, 2024
@DanSheps
Copy link
Member Author

DanSheps commented Aug 16, 2024

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.

Copy link
Contributor

github-actions bot commented Sep 1, 2024

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 github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 1, 2024
@DanSheps DanSheps removed the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 12, 2024
Copy link
Contributor

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 github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Oct 12, 2024
@DanSheps DanSheps removed the pending closure Requires immediate attention to avoid being closed for inactivity label Oct 15, 2024
if len(terminations) > 1:
assert all(t.link == terminations[0].link for t in terminations[1:])
if len(terminations) > 1 and not all(t.link == terminations[0].link for t in terminations[1:]):
raise UnsupportedCablePath(_("All originating terminations must start must be attached to the same link"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the message content, the sentence is incorrect in its current state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when adding new connection to front port.
3 participants