starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

API deleting vlan associated with prefix causes HTTP error 500 #2629

New Issue
Closed
opened 2025-12-29 18:20:40 +01:00 by adam · 12 comments
adam commented 2025-12-29 18:20:40 +01:00
Owner
Copy Link

Originally created by @louis-6wind on GitHub (May 23, 2019).

Environment

  • Python version: 3.6.6
  • NetBox version: 2.5.12

Steps to Reproduce

  1. Add a vlan
  2. Add a prefix
  3. Associate vlan to prefix
  4. Delete vlan from API

Expected Behavior

  • Get correct HTTP reply
  • Dissociate vlan from prefix
  • Delete Vlan

Observed Behavior

fails with HTTP 500

Originally created by @louis-6wind on GitHub (May 23, 2019). ### Environment * Python version: 3.6.6 * NetBox version: 2.5.12 ### Steps to Reproduce 1. Add a vlan 2. Add a prefix 3. Associate vlan to prefix 4. Delete vlan from API ### Expected Behavior - Get correct HTTP reply - Dissociate vlan from prefix - Delete Vlan ### Observed Behavior fails with HTTP 500
adam added the type: bugstatus: accepted labels 2025-12-29 18:20:40 +01:00
adam closed this issue 2025-12-29 18:20:40 +01:00
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 23, 2019):

This is due to the relation having models.PROTECT set as on_delete. There are two ways this could be made clearer, semantics-wise.

  1. The simpler way would be to change that to models.CASCADE, which would presumably behave as you describe.
  2. The more complicated way would be to guard on errors raised by the protection (the error to be caught is ProtectedError), and provide a more helpful error message.

I’m not sure which way is preferrable.

@hellerve commented on GitHub (May 23, 2019): This is due to the relation having `models.PROTECT` set as `on_delete`. There are two ways this could be made clearer, semantics-wise. 1. The simpler way would be to change that to `models.CASCADE`, which would presumably behave as you describe. 2. The more complicated way would be to guard on errors raised by the protection (the error to be caught is `ProtectedError`), and provide a more helpful error message. I’m not sure which way is preferrable.
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@yarnocobussen commented on GitHub (May 24, 2019):

@hellerve
Wouldn't using cascade also delete the prefix?
https://docs.djangoproject.com/en/2.2/ref/models/fields/

@yarnocobussen commented on GitHub (May 24, 2019): @hellerve Wouldn't using cascade also delete the prefix? https://docs.djangoproject.com/en/2.2/ref/models/fields/
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 24, 2019):

It would. It turns out I miread OP’s desired behavior. The behavior that they want is actually probably models.SET_NULL.

@hellerve commented on GitHub (May 24, 2019): It would. It turns out I miread OP’s desired behavior. The behavior that they want is actually probably `models.SET_NULL`.
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@DanSheps commented on GitHub (May 24, 2019):

Yes, we wouldn't want to use Cascade here. SET_NULL is what you want.

@DanSheps commented on GitHub (May 24, 2019): Yes, we wouldn't want to use Cascade here. SET_NULL is what you want.
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 24, 2019):

@DanSheps do you want to work on it? Otherwise I’m fine with adding a patch!

@hellerve commented on GitHub (May 24, 2019): @DanSheps do you want to work on it? Otherwise I’m fine with adding a patch!
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 24, 2019):

Sorry if I stole your thunder by opening #3215 there.

@hellerve commented on GitHub (May 24, 2019): Sorry if I stole your thunder by opening #3215 there.
adam commented 2025-12-29 18:20:41 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (May 27, 2019):

The PROTECT behavior was selected for a reason and needs to remain in place to avoid the accidental destruction of data. The scope of this bug is limited to handling the exception.

@jeremystretch commented on GitHub (May 27, 2019): The `PROTECT` behavior was selected for a reason and needs to remain in place to avoid the accidental destruction of data. The scope of this bug is limited to handling the exception.
adam commented 2025-12-29 18:20:42 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 27, 2019):

I can fix it in that way instead. Would you be interested in a PR from my side?

Generally, we could introduce a middleware that catches all instances of ProtectedError and returns more informative/semantically pleasing error messages. This would avoid repeting that kind of handler over and over; the other side of that equation is that a middleware might be a little heavyweight for that purpose!

@hellerve commented on GitHub (May 27, 2019): I can fix it in that way instead. Would you be interested in a PR from my side? Generally, we could introduce a middleware that catches all instances of `ProtectedError` and returns more informative/semantically pleasing error messages. This would avoid repeting that kind of handler over and over; the other side of that equation is that a middleware might be a little heavyweight for that purpose!
adam commented 2025-12-29 18:20:42 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (May 27, 2019):

@hellerve I was going to dig into it a bit later, but you're certainly welcome to take a stab at it. I'd like to avoid introducing new middleware, but I haven't evaluated any other options.

@jeremystretch commented on GitHub (May 27, 2019): @hellerve I was going to dig into it a bit later, but you're certainly welcome to take a stab at it. I'd like to avoid introducing new middleware, but I haven't evaluated any other options.
adam commented 2025-12-29 18:20:42 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (May 28, 2019):

To provide a bit of context: The ExceptionHandlingMiddleware was introduced to catch common installation/configuration issues and draw the user's attention to them, in an effort to shorten the troubleshooting process. It's not intended to address issues stemming from the data itself.

Here's how we currently handle ProtectedError exceptions in ObjectDeleteView:

try:
    obj.delete()
except ProtectedError as e:
    handle_protectederror(obj, request, e)
    return redirect(obj.get_absolute_url())

handle_protectederror() extracts the dependent object(s) from the exception and pushes a human-friendly error onto the session's messages stack. The original view is then allowed to complete normally, but without the object being deleted.

I figure the simplest way to replicate this behavior for the API would be to extend ModelViewSet to also catch ProtectedError and return an appropriate error code and message. (I'd argue against a 403 since that usually implies a permissions problem; maybe 409?)

@jeremystretch commented on GitHub (May 28, 2019): To provide a bit of context: The [ExceptionHandlingMiddleware](https://github.com/digitalocean/netbox/blob/develop/netbox/utilities/middleware.py#L44) was introduced to catch common installation/configuration issues and draw the user's attention to them, in an effort to shorten the troubleshooting process. It's not intended to address issues stemming from the data itself. Here's how we currently handle `ProtectedError` exceptions in [`ObjectDeleteView`](https://github.com/digitalocean/netbox/blob/develop/netbox/utilities/views.py#L288): ``` try: obj.delete() except ProtectedError as e: handle_protectederror(obj, request, e) return redirect(obj.get_absolute_url()) ``` [`handle_protectederror()`](https://github.com/digitalocean/netbox/blob/develop/netbox/utilities/error_handlers.py#L6) extracts the dependent object(s) from the exception and pushes a human-friendly error onto the session's messages stack. The original view is then allowed to complete normally, but without the object being deleted. I figure the simplest way to replicate this behavior for the API would be to extend [`ModelViewSet`](https://github.com/digitalocean/netbox/blob/develop/netbox/utilities/api.py#L225) to also catch `ProtectedError` and return an appropriate error code and message. (I'd argue against a 403 since that usually implies a permissions problem; maybe 409?)
adam commented 2025-12-29 18:20:42 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 28, 2019):

@jeremystretch Sure, we can do that! I’m lacking a little context in some parts of the codebase, so bear with me when I reach for the wrong abstraction every once in a while! I’ll adjust #3222.

@hellerve commented on GitHub (May 28, 2019): @jeremystretch Sure, we can do that! I’m lacking a little context in some parts of the codebase, so bear with me when I reach for the wrong abstraction every once in a while! I’ll adjust #3222.
adam commented 2025-12-29 18:20:42 +01:00
Author
Owner
Copy Link

@hellerve commented on GitHub (May 28, 2019):

@jeremystretch I moved the logic into ModelViewSet.dispatch and adjusted the status code to 409.

@hellerve commented on GitHub (May 28, 2019): @jeremystretch I moved the logic into `ModelViewSet.dispatch` and adjusted the status code to `409`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label status: accepted type: bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#2629
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?