ModelAdmin: Add attribute to override manager used by ModelAdmin.get_queryset()

[Jon Dufresne]

Use case:

Suppose I have a soft-deletable model. Throughout the application, I want queries to exclude deleted objects. I do this by overriding the model's default manager. In the admin, I'd like to be able to edit all objects in order to undelete them. To do this I might do:

class ArticleManager(models.Manager):
     def get_queryset(self):
         return super().get_queryset().filter(deleted=False)

 class Article(models.Model):
     ...
     deleted = models.BooleanField(default=False)
     objects = ArticleManager()

@admin.register(Article)
class ArticleAdmin(admin.ModelAdmin):
    def get_queryset(self):
        return Article._base_manager.all()

However, ModelAdmin.get_queryset() is also responsible for ​applying the ordering. From django/contib/admin/options.py:

    def get_queryset(self, request):
        qs = self.model._default_manager.get_queryset()
        ordering = self.get_ordering(request)
        if ordering:
            qs = qs.order_by(*ordering)
        return qs

So to override this method to use a different manager requires duplicating the ordering logic.

Solution:

I think this can be improved to avoid the duplication by specifying the manager as an attribute on ModelAdmin. For example, something like:

@admin.register(Article)
class ArticleAdmin(admin.ModelAdmin):
    manager_name = '_base_manager'

[Jon Dufresne]

​https://github.com/django/django/pull/10318

[Carlton Gibson]

I'm not sure about this.

(Slight aside, I'm not sure that soft-delete isn't an anti-pattern but) my understanding of this was always that if you do implement these custom managers, you also need to make sure you don't interfere with exactly this kind of internal behaviour.

From the docs:

If you use custom Manager objects, take note that the first Manager Django encounters (in the order in which they’re defined in the model) has a special status. Django interprets the first Manager defined in a class as the “default” Manager, and several parts of Django (including dumpdata) will use that Manager exclusively for that model. As a result, it’s a good idea to be careful in your choice of default manager in order to avoid a situation where overriding get_queryset() results in an inability to retrieve objects you’d like to work with.

The examples in the docs point to this sort of thing:

    objects = models.Manager() # The default manager.
    dahl_objects = DahlBookManager() # The Dahl-specific manager.

... where we leave the built-in functionality alone and implement our base filtering (i.e. soft-delete here) in a separate manager.

It seems to me that adding API to ModelAdmin (or needing to override get_queryset() to go via _base_manager) in order to allow correcting for not following the advice here is... well... not the way I'd go, shall we say.

Not totally against, but that's my thought.

[Jon Dufresne]

Thanks for the feedback Carlton. On the projects I work on, I've found the soft delete pattern to be very valuable and successful.

I see the trade off as follows:

Pros

• Never mistakenly display a deleted object (big one)
• Accessing deleted objects is very explicit as a different manager is used

Cons

• Must override some Django components to use a different manager (e.g. ModelAdmin.get_queryset())

On the project I work on with sensitive data, the "Never mistakenly display a deleted object" outweighs the other considerations. So much that I don't mind overriding the cases that need access to deleted objects.

On a growing team with knowledge gaps made up of diverse skill levels, it is sometimes hard to always make sure that all code uses the correct manager. Having the default manager act correct for a large majority of cases and never mistakenly display deleted data goes a long way to preventing bugs getting to users. On my project, the introduction of this pattern sometimes comes immediately after such a bug.

When writing new code it is just too easy and common to reach for MyModel.objects... but one wants MyModel.active_objects... or MyModel.objects.filter(deleted=False). As a result, coding mistakes happen and bugs get to users.

After reading the quoted text, I think the advice comes down to "be careful" not "don't do this". I understand the trade offs I'm making.

[Carlton Gibson]

Hey Jon.

Yes. I'm quite sure you do understand the trade-offs. That doesn't convince me that we should be encouraging (or enabling even) users down this road.

Forcing you to override get_queryset(), duplicated logic isn't much:

    def get_queryset(self, request):
        qs = self.model._base_manager.get_queryset()
        ordering = self.get_ordering(request)
        if ordering:
            qs = qs.order_by(*ordering)
        return qs

Half-a-dozen lines seems better to me that adding an extra option to the ModelAdmin API. (Which is already massive.)

(And once we're implementing get_queryset() if we skip ordering and just add an orderBy it'll be 2 lines, which you save not having ordering and, the proposed, manager_name anyway.)

[Jon Dufresne]

I appreciate the desire to contain the number of ModelAdmin configuration options. However, this option is very well contained, simple, and solves a real use case for me. I don't think it will add much overhead or ongoing maintenance.

I agree that to override ModelAdmin.get_queryset() that I'd be copying just a small handful of lines. I still don't think it is a great practice. It ignores that the Django ModelAdmin could slowly evolve with features while my overridden methods don't not call the parent. You're now suggesting that I customize more logic of ModelAdmin.get_queryset(), I don't consider that an improvement when it could be handled with a declarative class attribute.

That doesn't convince me that we should be encouraging (or enabling even) users down this road.

What if it remained an undocumented API? That would be enough for me to use the feature, but might handle this concern for you.

[Carlton Gibson]

I think in this case overriding get_queryset() leads to simpler, more straightforward code, that more clearly expresses your intent, and is ultimately more maintainable.
Even if you have to write a few more lines. Basically I think we're well beyond the sweet spot of these kind of declarative options here. (But this isn't something we need to agree on now.)

My preferred option here is very much to not add extra API. (But mine is just one opinion.)

What if it remained an undocumented API?

Well that's one idea. :-)

If we add it, there's no problem with it being documented but I think it should just say something along the lines of...

.. attribute:: ModelAdmin.manager_name

     Default, ``'_default_manager'``. Set this to the name of the model's manager to be used
     by :meth:`.ModelAdmin.get_queryset()` if the default manager is not appropriate for your needs. 

i.e. nice and vague, but there if you already know what all the terms mean.

If we have an example then I think that should be of the DahlManager type, i.e. creating an Admin restricted to a filtered queryset, not one showing the very pattern we warn against, where you filter the default queryset in the default manager.

(Why not that? Because people will be doing ​Tutorial 2, see this pattern in the Admin docs, put it in their code, and give themselves all sorts of trouble.)

[Jon Dufresne]

I have updated the PR with your suggested doc changes. They work fine for me.

While I still feel this change is an improvement for my use case, I'll be fine if this ticket get resolved as "wontfix". I'll just continue to regularly override ModelAdmin.get_queryset() as needed.

[Carlton Gibson]

OK. Let’s move this forward.

The patch itself is fine. We’ll ask Tim to make the call on whether to go with it.

Thanks Jon

[Carlton Gibson]

Right, Tim seems to be studiously ignoring this one — which is 100% fair, since there's no reason he has to make all the decisions.

As such, I'm going to say wontfix to this one.

1. In general the cognitive overload on the ModelAdmin API is already high-enough, and I'm predisposed against adding more API there.
2. In particular I think the supposed benefit accrued from having a declarative attribute is minimal at best and users are better-off overriding get_queryset() directly. (The purpose of get_queryset() is clear-as-day. The function of a manager_name is not. Thus in the latter case I'm off to the docs.)

Jon, I hope you understand these points, even if you don't agree with them. I'm very happy if you want to pick it up on django-developers to discuss.
(It might be interesting to sound out general opinions on the ModelAdmin API.)

[Tim Graham]

For the record, my inaction wasn't meant to convey an opinion. I was merely occupied with some other priorities.

[Carlton Gibson]

Replying to Tim Graham:

For the record, my inaction wasn't meant to convey an opinion. I was merely occupied with some other priorities.

Yes. Of course. My comment wasn't meant to imply otherwise. Merely that it's not reasonable to expect you always to be the arbiter.
Thanks Tim!