[Django] #29688: ModelAdmin: Add attribute to override manager used by ModelAdmin.get_queryset()
Django
ModelAdmin.get_queryset()
-----------------------------------------+------------------------
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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
[https://github.com/django/django/blob/3daac76cfbb55acb57c9a8bbfa6f12f766eacc3f/django/contrib/admin/options.py#L349-L359
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'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29688>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/10318
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:1>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 override 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`) 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:2>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:3>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:4>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:5>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 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
[https://docs.djangoproject.com/en/2.1/intro/tutorial02/ Tutorial 2], see
this pattern in the Admin docs, put it in their code, and give themselves
all sorts of trouble.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:6>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:7>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Ready for checkin
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:8>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:9>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
For the record, my inaction wasn't meant to convey an opinion. I was
merely occupied with some other priorities.
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:10>
Django
ModelAdmin.get_queryset()
Reporter: Jon Dufresne | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Replying to [comment:10 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29688#comment:11>