Using a custom default manager can lead to un-editable objects in admin

[James Bennett]

When a model's default manager overrides get_query_set() with anything which filters out certain objects, then those objects will be uneditable via the admin app regardless of which manager is specified for the admin to use. This is because the admin change_stage view uses the automatic ChangeManipulator for the model, which in turn uses the model's default manager to fetch the object to change. So if the model's default manager happens to filter in a way which excludes that object from the returned QuerySet, the ChangeManipulator will raise ObjectDoesNotExist, which in turn causes the admin to return a 404.

The only thing I can think of to solve this would be for the admin to use its own ChangeManipulator class which uses the manager specified in the model's inner Admin class to fetch the object to edit.

[Malcolm Tredinnick <malcolm@…>]

The problem sounds annoying, but the solution sounds worse. If you have a custom ChangeManipulator, it means that the default one does not do the job. This could be because you need to check constraints on other objects, or do some updating elsewhere or something similar. Falling back to the default ChangeManipulator in all cases will be very dangerous.

We might need to do something like special-case the situation for objects that are not returned via the model's get_query_set(), rather than for all objects. Needs more thinking, because there might be other cases we haven't thought of, too.

[James Bennett]

The problem is extremely annoying, because I imagine that as the new trunk starts getting used this will be a common use case.

This might not be the most elegant solution, but if a model has an Admin class and specifies a manager for the admin, how about giving it an automatic AdminChangeManipulator (which would be a subclass of AutomaticManipulator) which would use the correect manager?

[Malcolm Tredinnick <malcolm@…>]

Does it make sense that if you have a customer ChangeManipulator, that should be used wherever possible? My logic is that this manipulator knows all the business logic and you shouldn't be able to work around that (accidently or intentionally) just because you are using the admin interface. It would seem to me (at first glance, at least), that only if the current object does not appear in get_query_set() should you revert to using something like an AdminChangeManipulator.

[James Bennett]

I could see it going either way; falling back to an AdminChangeManipulator if the object isn't found would be a complicated, but viable option. But so would advertising the fact that if you use a custom ChangeManipulator which overrides the default one for the model, you should either also override the AdminChangeManipulator or ensure that the model's default manager won't hide any objects.

[Malcolm Tredinnick <malcolm@…>]

I was thinking about this a bit more. Are we making the problem sound harder than it really is? The documentation (model-api.txt) already mentions the problems that can result in the admin interface from changing get_query_set(). It also points out the solution: add in a custom Manager to the class that provides a more expansive get_query_set() method. The admin interface uses the first manager defined, no matter what it is called.

It may not be necessary to go so far as creating a new ChangeManipulator. Can you just put

   admin_manager = models.Manager()
   objects = CustomManager()

in your class (in that order)? If so, then I suspect this does not require any change to Django, since it's documented (we could maybe tweak the documentation with an example).

Note: I have not tested this too thoroughly. There may be some reason the whole manipulator needs replacing. But your description sounds like you have tracked it down to get_query_set() (and the documentation supports that analysis).

[James Bennett]

It does work if you do that, but then you'll run into problems with related objects, because fetching across relations uses the default manager. To give a concrete example, I'm working on a blog app where blog entries can be 'published' or 'draft', so for public views I want a manager which only returns published entries. If I let models.Manager() be the default manager, then I lose some of the utility of this because fetching entries from a related object (in this case, a Category) will use that manager, making it necessary to filter draft entries out of the returned QuerySet. But if I let the custom published-only manager be the default manager, then draft entries aren't editable in the admin regardless of which manager the admin is told to use.

Either way, some of the utility of the custom manager is lost.

[Luke Plant]

The following is how I've 'solved' this problem, but I realise that some of the details means it won't apply that well to other situations.

I use an approach like ticket #1268 to store the User (and also a custom 'Member' object) in threadlocal storage (see ​http://lukeplant.me.uk/blog.php?id=1107301634 for something more like my actual code). Only staff members are 'Users', public users of the site are 'Members', but the solution doesn't depend on this -- you could distinguish between 'Users' and 'Users' who have is_staff=True, for instance. My overridden get_query_set() method then accesses the User and/or Member object to decide what filters to add e.g. 'posts' that are 'hidden' are not visible to 'Members', only 'Users', private 'Messages' are only visible to the Member they are to etc.

This works beautifully, and can include business rules as complex as you like, and they all work in a 'related' context. I usually have another (non-default) manager that returns all objects, for the very few cases where business logic requires it.

The main disadvantage of this method (which is not a disadvantage in my case) is that in the public views, admin users who are logged in will still see the 'hidden' objects (I just put a visible 'this is hidden' marker against those items, which is OK for me).

Another possibility I've just thought of is a middleware that sticks an 'isadminrequest' flag into threadlocal storage (the flag can usually be determined pretty easily from the URL). The get_query_set() method can then use this to decide on filters. This is more hacky IMO than the method outlined above, but on the other hand, it will work very nicely AFAICS, and involves minimal disturbance to existing code, and could therefore be regarded as quite elegant. If you squint in the right way ;-)

[Gary Wilson <gary.wilson@…>]

Maybe this could be fixed with the new admin options stuff. Also removed keyword spam.

[James Bennett]

This isn't going to be relevant as of newforms-admin, and I'm the one who originally opened this, so I'm closing it invalid now.