Opened 10 years ago

Closed 8 years ago

Last modified 5 years ago

#2145 closed defect (fixed)

list_filter not available if model contains OneToOneField

Reported by: Daniel Roseman Owned by: nobody
Component: contrib.admin Version: master
Severity: normal Keywords: OneToOneField ModelInheritance MultiTableInheritance editable parent_link
Cc: gary.wilson@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If a model is related to via a OneToOneField (even if it's not used in the admin list), the list_filter block doesn't appear on the admin list page.

The offending code appears to be in django.contrib.admin.views.main

571    def get_filters(self, request):
572        filter_specs = []
573        if self.lookup_opts.admin.list_filter and not self.opts.one_to_one_field:
574            filter_fields = [self.lookup_opts.get_field(field_name) \
575                              for field_name in self.lookup_opts.admin.list_filter]

Commenting out "and not self.opts.one_to_one_field" in line 573 reinstates the functionality and appears to have no ill effects. I don't know what it is supposed to be testing for, though, so don't know if it breaks something somewhere else.

Attachments (3)

patch.txt (667 bytes) - added by alex@… 10 years ago.
trivial patch
djt-2145-one_to_one_field-contribution.diff (958 bytes) - added by Jeremy Dunck <jdunck@…> 10 years ago.
Contribute to restaurant, not place.
2145-one-to-one-field_editable.diff (610 bytes) - added by Tyson Clugg 8 years ago.
Patch to allow editing OneToOneFields

Download all attachments as: .zip

Change History (37)

comment:1 Changed 10 years ago by anonymous

Cc: gary.wilson@… added

comment:2 Changed 10 years ago by remco@…

Any changes concerning this ticket? I still get the feeling that though for some relationships OneToOneField is conceptually nicer to use, one is better off using a ForeignKey field within django.

comment:3 Changed 10 years ago by anonymous

Resolution: fixed
Status: newclosed

comment:4 Changed 10 years ago by James Bennett

Resolution: fixed
Status: closedreopened

Please don't modify ticket status anonymously, and if you close a ticket leave a note explaining the rationale.

comment:5 Changed 10 years ago by Gary Wilson <gary.wilson@…>

Triage Stage: UnreviewedAccepted

comment:6 Changed 10 years ago by alex@…

We have a absolutely duplicate ticket:2718 on the same issue with same patch suggested.
I hope nobody will be angry on me, I closed ticket:2718 and kept this one as being "earlier" :)

This is very trivial fix and I have to maintain it on all my Django powered severs. I am voting to push this change into
main tree.

comment:7 Changed 10 years ago by bram.dejong+django@…

I experience the same trouble, and independently found the same fix... When is this planned to be included in trunk?

comment:8 Changed 10 years ago by Simon G. <dev@…>

I'll move this to ready to checkin once someone works up a patch.

--Simon

comment:9 Changed 10 years ago by Simon G. <dev@…>

Needs tests: set
Patch needs improvement: set

...and some tests would be nice too.

Changed 10 years ago by alex@…

Attachment: patch.txt added

trivial patch

comment:10 Changed 10 years ago by alex@…

I've attached the trivial patch which I am using last months on all django installs.

comment:11 Changed 10 years ago by Jeremy Dunck <jdunck@…>

Has patch: set

I've tried to work up a test, but have run into trouble due to tight coupling.

I think it may be possible using the web client, but a simple model-based regression test doesn't seem possible. It looks like no other tests require the web client, so I'm reluctant to add the first one.

I think the trivial patch given is wrong, in that it misses the point. I think the real problem is that one_to_one_rel is being contributed to the related class, e.g. "place" when it ought to be contributed to the referencing class (e.g. restaurant). I think one_to_one_field is basically just supposed to be a well-known name, like self.pk.

I'll attach a different patch in a moment, but note the patch solved two problems for me: this one, and the fact that limit_choices_to on Restaurant was causing the *Place* change list to be filtered based on the limit_choices_to kwargs.

This pointed me in what I think is the right direction: one_to_one_field should be contributed to restaurant, not place.

All existing tests still pass with the new patch.

Changed 10 years ago by Jeremy Dunck <jdunck@…>

Contribute to restaurant, not place.

comment:12 Changed 10 years ago by Ilya Semenov <semenov@…>

+1 on this patch, just run into the same issue and did the same fix. I see absolutely no rationale behind this limitation (list_filter disabled if there's at least one OneToOneField).

comment:13 Changed 9 years ago by sgt.hulka@…

What needs to be done to push this into trunk?

comment:14 Changed 9 years ago by Jos@…

I've run into the same problem, waiting for this to be pushed to trunk.

comment:15 Changed 9 years ago by anonymous

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Hmm.. not sure why I was so eager for tests. I've moved this on, and thanks for reminding me :)

comment:16 Changed 9 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Something's not right about this patch. With current trunk ([5983]) and the following two models:

from django.db import models

class Place(models.Model):
    name = models.CharField(max_length=50)

    def __unicode__(self):
        return u"%s the place" % self.name

class Restaurant(models.Model):
    place = models.OneToOneField(Place)
    serves_hot_dogs = models.BooleanField()
    serves_pizza = models.BooleanField()

    class Admin:
        list_display = ('place', 'serves_hot_dogs', 'serves_pizza')
        list_filter = ('serves_hot_dogs', 'serves_pizza')

    def __unicode__(self):
        return u"%s the restaurant" % self.place.name

the list filtering displays and works correctly without the patch and is broken when the patch is applied. Current behaviour looks to be correct for this particular example case.

On the surface, Jeremy's logic in comment 11 looks correct. In practice, something is going wrong.

comment:17 Changed 9 years ago by Peter Baumgartner

Resolution: worksforme
Status: reopenedclosed

list_filter appears to be working in current svn with OneToOneField. Jeremy's patch no longer breaks Malcolm's example above.

Neither svn nor Jeremy's patch fix the other issue with limit_choices_to.

comment:18 in reply to:  17 Changed 9 years ago by anonymous

Filed ticket #5497 for limit_choices_to bug

comment:19 Changed 9 years ago by jos@…

Resolution: worksforme
Status: closedreopened

I still see this problem in svn (revision 6463)
It can be recreated with the following model:

from django.db import models

class System(models.Model):
    name = models.CharField(maxlength=255)

    class Admin:
        list_filter = ( 'name',  )

class SystemDeploy(models.Model):
    system=models.OneToOneField(System)
    currentversion = models.IntegerField()

When system=models.OneToOneField(System) is commented the filters show up.

The initial patch created by alex works for me

comment:20 Changed 9 years ago by Jeremy Dunck

FWIW, I've stopped using OneToOneField in favor of ForeignKey(unique=True, related_name='<some singular>_one').

OneToOneField is to be dropped some day, if I remember the mail thread correctly (can't find it now).

I think it's a bug that ForiegnKey(unique=True) results in a related queryset rather than a single object descriptor. But that's another matter.

For now, this works (omitted as before):

class Restaurant(models.Model):
    place = models.ForeignKey(Place, unique=True, related_name='restaurant_one')
    ....
...
>>> p=Place.objects.all()[0]
>>> r = p.restaurant_one.all()[0]

A bit ugly, but not buggy.

comment:21 Changed 9 years ago by Ilya Semenov

I don't think the 1-to-1 relation will be dropped away. It's one of the fundamental database concepts, and officially replacing it with unique-1-t-m would be just plain wrong (since in many times that's like replacing O(1) complexity to O(N)).

Still hoping for the bugfix to be applied. I'm also wondering why the 1-to-1 problem was not arised during September Sprint - personally, I forgot about it, using the proposed patch and "empty" records instead of 1-to-1 NULLs for several months.

comment:22 Changed 9 years ago by Jeremy Dunck

@semenov:

The fact that it didn't rise to attention in the Sept sprint suggests that not that many people (particularly core committers) are using OneToOne.

As for 1-1 being cheaper than 1-m, in practical application, accessing the descriptor of a 1-1 field goes and fetches the related record by it's primary key, as does a FK descriptor access. Even if you're talking about querying across the table, it usually goes something like Related.objects.get(o2osomeattribute='x'), which IIRC, Django doesn't munge into a subquery, but rather uses a join anyway. (Arguably another bug.)

The fact is that 1-1 is an edge case in lots of code, and fixing the bugs isn't very easy. It needs a champion for it to live, I think.

(I'm not a committer, but my tea-reading skills are fairly good.)

comment:23 Changed 9 years ago by remco@…

"The semantics of one-to-one relationships will be changing soon, so we don’t recommend you use them. If that doesn’t scare you away, keep reading."

-- http://www.djangoproject.com/documentation/model-api/#one-to-one-relationships

@jdunck: I don't know exactly how the 1-1 not being part of the sprint would suggest that not too many people are using it, as I can equally well state that maybe there was a lot of other stuff to improve and change during the sprint. But I agree with semenov that 1-1 relationships is a fundamental part of defining relationships in data models. So I for one hope this issue get's fixed over time.

comment:24 Changed 9 years ago by Ilya Semenov

As for 1-1 being cheaper than 1-m, in practical application, accessing the descriptor of a 1-1 field goes and fetches the related record by it's primary key, as does a FK descriptor access

That is only correct in the current (broken) implementation, where a 1-to-1 relation effectively can not have null=True (well you can describe it as NULL, but the ORM code would break on fetching, and I didn't manage to find a simple way to fix that). Once the 1-to-1 is (hopefully) fixed to allow NULL values, it becomes zero time to fetch a missing 1-to-1 value, and a separate database lookup to fetch a missing unique-1-to-m value.

Besides of that, it degrades the code readability to have a collection rather than a value where it just doesn't make sense. For example, consider User and Profile models:

if user.profile:
 phone = user.profile.phone
else:
 phone = DEFAULT_PHONE

The 1-to-m approach would lead to unneeded overhead (in terms of both effeciency and the code clarity):

if user.profile_set.count():
 phone = user.profile_set.get().phone
 # and you always pray it doesn't crash with an AssertionError for 2+ objects
else:
 phone = DEFAULT_PHONE

comment:25 Changed 9 years ago by alex@…

This discussion needs to be moved to django-developers, because otherwise we add too much unrelated theoretical information to ticket.

comment:27 Changed 8 years ago by Gary Wilson

milestone: 1.0

comment:28 Changed 8 years ago by Tyson Clugg

Keywords: OneToOneField ModelInheritance editable parent_link added

I've just experienced similar problems in trunk (now that the ModelInheritance branch has been merged).

I noticed that the OneToOneField.init has kwargseditable?=False hard coded, which breaks how we were using OneToOneFields. The patch I'm submitting restores OneToOneField behaviour to pre ModelInheritance status by checking if the field has parent_link=True before setting editable=False.

Changed 8 years ago by Tyson Clugg

Patch to allow editing OneToOneFields

comment:29 Changed 8 years ago by Tyson Clugg

My last comment and the patch I submitted do not belong on this ticket - I should have created a new ticket (the patch applies to a new defect). The new ticket (#7947) has been created, please disregard the patch and comment of mine above.

comment:30 Changed 8 years ago by anonymous

Keywords: MultiTableInheritance added

Because multi-table inheritance is achieved using implicit OneToOneFields, any non-abstract parent model classes will not have the list_filter functionality:

class Place(models.Model):
    name = models.CharField(max_length=50)
    city = models.CharField(max_length=50)

class Restaurant(Place):
    serves_pizza = models.BooleanField()

In the example above, Restaurant will have filter functionality, but Place won't!

This issue needs to be resolved!!!

comment:31 Changed 8 years ago by Brian Rosner

I am going to commit the original patch. There has been a ton of refactoring since the queryset-refactor branch landed completely overhualing OneToOneFields. I have tested the first patch and it works. I would rather close this as fixed with that change (to me it doesn't seem correct to limit the filtering on that condition). Then if any new bugs surface lets clean those up in new tickets.

comment:32 Changed 8 years ago by Brian Rosner

Resolution: fixed
Status: reopenedclosed

(In [8388]) Removed some checks for Model._meta.one_to_one_field to prevent list_filter and the show_result_count in search_form.html. Fixes #2145.

comment:33 Changed 8 years ago by Brian Rosner

For the record, that should have read "that prevented" and not "to prevent" :)

comment:34 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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