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.

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)