Code

Opened 8 years ago

Closed 6 years ago

Last modified 3 years ago

#2145 closed defect (fixed)

list_filter not available if model contains OneToOneField

Reported by: danielr 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@… 7 years ago.
trivial patch
djt-2145-one_to_one_field-contribution.diff (958 bytes) - added by Jeremy Dunck <jdunck@…> 7 years ago.
Contribute to restaurant, not place.
2145-one-to-one-field_editable.diff (610 bytes) - added by tyson 6 years ago.
Patch to allow editing OneToOneFields

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by anonymous

  • Cc gary.wilson@… added

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

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

comment:4 Changed 7 years ago by ubernostrum

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

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

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 7 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 7 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 7 years ago by Simon G. <dev@…>

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

--Simon

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

  • Needs tests set
  • Patch needs improvement set

...and some tests would be nice too.

Changed 7 years ago by alex@…

trivial patch

comment:10 Changed 7 years ago by alex@…

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

comment:11 Changed 7 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 7 years ago by Jeremy Dunck <jdunck@…>

Contribute to restaurant, not place.

comment:12 Changed 7 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 7 years ago by sgt.hulka@…

What needs to be done to push this into trunk?

comment:14 Changed 7 years ago by Jos@…

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

comment:15 Changed 7 years ago by anonymous

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 follow-up: Changed 7 years ago by baumer1122

  • Resolution set to worksforme
  • Status changed from reopened to closed

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 7 years ago by anonymous

Filed ticket #5497 for limit_choices_to bug

comment:19 Changed 7 years ago by jos@…

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 7 years ago by jdunck

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 7 years ago by 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 7 years ago by jdunck

@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 7 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 7 years ago by 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 7 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 6 years ago by gwilson

  • milestone set to 1.0

comment:28 Changed 6 years ago by tyson

  • 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 6 years ago by tyson

Patch to allow editing OneToOneFields

comment:29 Changed 6 years ago by tyson

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 6 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 6 years ago by brosner

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 6 years ago by brosner

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 6 years ago by brosner

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

comment:34 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.