Django Unified Method for Deleting Instances of Any Model

Question

This code works, but I don't know if there are better and especially safer ways to do this.

For context: this is a Django view (Python) and I wanted to unify the deletion methods because there were many of them.

def delete(request, class_name, object_ID):
 obj = get_object_or_404(eval(class_name), id=object_ID)
 obj.delete()
 return redirect(dashboard)

Now I know that the eval() method should be used with caution, but would it be enough to wrap a try-catch-method around it or should I do something like a switch-case, or is it even ok to use the code as is?

It will be called from this url route (urls.py):

path('delete/<str:className>/<int:object_ID>', views.delete)

Note: This route will only be accessible to employees and there will be a permission check, so my question is about best practice and potential problems.

• 301_Moved_Permanently: While I believe the question is answerable as-is, I think it would add value and help answer the "ok to use" question if you included a few examples of how you call this function in your views. Also, how is dashboard defined?
• creyD: @409_Conflict The dashboard view is just another view with no parameters, I can't imagine how it could cause problems. I am calling this view directly from the urls.py and added an example above.

Answer 1

The use of eval() here is less than ideal as it allows you to call the URL delete/User/3/, for instance, and it will work against all odds. Worse, you can call the URL delete/delete/9 where eval will resolve delete to this view function and lead to all kind of weird behavior.

So the first step is to restrict which symbols can be found from there. The easiest way is to retrieve the attribute directly from your models:

from . import models
...
def delete(request, type_name, object_id):
 try:
 cls = getattr(models, type_name)
 except AttributeError:
 raise Http404
 get_object_or_404(cls, id=object_id).delete()
 redirect(dashboard)

But this is still too large as anything that you imported in models can still be retrieved. So you're better off using a base class in your models.py that only your deletable models will inherit from:

class Deletable:
 def can_delete(self, user):
 return user.is_active and user.is_staff
class OneOfYourCustomModel(Deletable, models.Model):
 # whatever

And check in your view that whatever you get is Deletable:

def delete(request, type_name, object_id):
 try:
 cls = getattr(models, type_name)
 except AttributeError:
 raise Http404
 if inspect.isclass(cls) and issubclass(cls, models.Deletable):
 get_object_or_404(cls, id=object_id).delete()
 else:
 raise Http404
 redirect(dashboard)

You'll also note that it also lead to easy customization of which user can delete what: you just need to override one method and add proper logic (eg raising 403 forbidden) in your view. This can be more useful than the @staff_member_required in case your models can be associated to (owned by) a particular user:

class MyOwnedModel(Deletable, models.Model):
 owner = models.ForeignKey(django.contrib.auth.models.User, ...)
 def can_delete(self, user):
 return user == self.owner or super().can_delete(user)

But of course, if only the staff can delete things, the decorator is plenty sufficient.

---

Lastly, it would be good to separate the "retrieve the actual model" logic from the "delete the particular instance of that model" one. The latter being the responsibility of the view, but the former would better fit as a custom URL converter.

The main advantage being that you don't need to handle the 404s for unknown models in your view, django will take care of that itself if your converter ever raises a ValueError. Going this route, you can have a converters.py:

from inspect import isclass
from django.urls.converters import StringConverter
from . import models
class DeletableConverter(StringConverter):
 def to_python(self, value):
 try:
 cls = getattr(models, value)
 except AttributeError:
 raise ValueError
 if not isclass(cls) or not issubclass(cls, models.Deletable):
 raise ValueError
 return cls
 def to_url(self, value):
 return value.__name__

Usage in your urls.py:

from django.urls import path, register_converter
from . converters import DeletableConverter
register_converter(DeletableConverter, 'deletable')
urlpatterns = [
 ...,
 path('delete/<deletable:model>/<int:object_id>', views.delete),
 ...,
]

And in your view:

def delete(request, model, object_id):
 instance = get_object_or_404(model, id=object_id)
 if instance.can_delete(request.user):
 # or a simple @staff_member_required decorator
 instance.delete()
 redirect(dashboard)

• creyD: Thanks a lot, that all sounds very good, I will try to implement this! Like I said in my post, I took care of the permission problem already, by using @login_required and @staff_member_required decorators for methods like this.
• 301_Moved_Permanently: @creyD I expanded a bit on how it should best be implemented. And added a discussion about the usecase of my approach compared to @staff_member_required.
• creyD: Thank you yet again for your detailed answer and your edits! :)