Opened 9 years ago

Closed 7 years ago

#1855 closed defect (invalid)

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

Reported by: ubernostrum Owned by: nobody
Component: contrib.admin Version: master
Severity: major Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Change History (13)

comment:1 Changed 9 years ago by ubernostrum

  • Summary changed from Using custom manager in admin can lead to un-editable objects depending on the model's default manager to Using a custom default manager can lead to un-editable objects in admin

comment:2 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by ubernostrum

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?

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by ubernostrum

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.

comment:6 Changed 9 years ago by 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).

comment:7 Changed 9 years ago by ubernostrum

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.

comment:8 Changed 9 years ago by lukeplant

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 ;-)

comment:9 Changed 9 years ago by hi-world cup

  • Cc hi-world cup added
  • Keywords rthml tab space editor js added
  • Summary changed from Using a custom default manager can lead to un-editable objects in admin to hi-world cup

comment:10 Changed 9 years ago by adrian

  • Cc hi-world cup removed
  • Summary changed from hi-world cup to Using a custom default manager can lead to un-editable objects in admin

comment:11 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:12 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Keywords rthml tab space editor js removed

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

comment:13 Changed 7 years ago by ubernostrum

  • Resolution set to invalid
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.
Back to Top