API deleting vlan associated with prefix causes HTTP error 500

[louis-6wind]

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

[hellerve]

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.

[yarnocobussen]

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

[hellerve]

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

[DanSheps]

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

[hellerve]

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

[hellerve]

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

[jeremystretch]

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.

[hellerve]

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!

[jeremystretch]

@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]

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?)

[hellerve]

@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]

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