Skip to content

SoftDelete is an anti-pattern #364

@danielquinn

Description

@danielquinn

My apologies if this comes across as grumpy. I don't mean to be disrespectful. I know what it's like to maintain a popular project and no one likes it when someone pipes up about how they don't like something.

With that said though, there's a very real problem here with the SoftDeletableModel.

Problem

The SoftDeletableModel (and corresponding manager) has been the bane of my life for years. I started one job and spent a year removing it from the codebase, only to start a new job and be faced with it again. The defaults it implements (overriding .objects) break internal operations with Django (specifically the admin) and leads to hundreds of lost hours trying to debug code that's silently hiding your own data from you.

Example

Consider this simple set of models:

class User(SoftDeletableModel):
    name = models.CharField(max_length=128)

class Purchase(models.Model):
    user = models.ForeignKey(User)
    note = models.TextField()

Now:

  1. Create a user and a purchase with that user.
  2. Soft delete that user
  3. Find the purchase in the admin.
  4. Try to save a change to the note field.

→ Django explodes because the attached user doesn't exist... except that it does exist but we've broken the defaults of the underlying framework by overriding .objects.all() to mean not actually all.

This problem reasserts itself when debugging: You're a new developer tasked with generating statistics based on the number of users, and using User.objects.all().count() to do it. For some reason, your math doesn't add up and you watch a few hours get sucked away by this anti-pattern. I have no idea what would happen in cases where you might do something like Purchase.objects.filter(user__name__startswith="..."), but I'm willing to bet it'll cause problems when comparing to User.objects.filter(name__startswith="...").

Please don't muck with the defaults.

Proposed Alternative

Rather than replacing .objects.all(), rework it to leave .all() alone and instead have .available(). Alternatively, leave objects alone altogether and just have an .available_objects manager on the class. In either case, explicit is better than implicit if for no other reason than there's no surprises.

Understandably, this would break current implementations using the SoftDeletableModel, but frankly not doing this means that more and more of this sort of thing will creep into code everywhere as this is a very popular library and this is being described as a good solution to a common problem. In its current implementation however it creates more problems than it solves.

At the very least, please add a warning to the documentation that using this model as your parent can create long-lasting and hard-to-remove problems with data integrity in your application and disruptions with core functions like the Django admin. I think that if the developers that came before me had seen such a message, they would have thought twice before using it as the default base for all models in the system.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions