Django

Code

Ticket #2145 (closed: fixed)

Opened 4 years ago

Last modified 2 years ago

list_filter not available if model contains OneToOneField

Reported by: danielr Assigned to: nobody
Milestone: 1.0 Component: django.contrib.admin
Version: SVN Keywords: OneToOneField ModelInheritance MultiTableInheritance editable parent_link
Cc: gary.wilson@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

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

patch.txt (0.7 kB) - added by alex@halogen-dg.com on 05/08/07 08:14:41.
trivial patch
djt-2145-one_to_one_field-contribution.diff (0.9 kB) - added by Jeremy Dunck <jdunck@gmail.com> on 05/11/07 19:48:39.
Contribute to restaurant, not place.
2145-one-to-one-field_editable.diff (0.6 kB) - added by tyson on 07/24/08 19:26:15.
Patch to allow editing OneToOneFields?

Change History

08/08/06 23:22:54 changed by anonymous

  • cc set to gary.wilson@gmail.com.

12/17/06 16:10:23 changed by remco@diji.biz

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.

02/15/07 10:24:54 changed by anonymous

  • status changed from new to closed.
  • resolution set to fixed.

02/15/07 10:51:52 changed by ubernostrum

  • status changed from closed to reopened.
  • resolution deleted.

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

02/27/07 12:51:07 changed by Gary Wilson <gary.wilson@gmail.com>

  • stage changed from Unreviewed to Accepted.

03/06/07 13:47:39 changed by alex@halogen-dg.com

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.

05/04/07 08:35:46 changed by bram.dejong+django@gmail.com

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

05/04/07 20:07:22 changed by Simon G. <dev@simon.net.nz>

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

--Simon

05/04/07 20:07:40 changed by Simon G. <dev@simon.net.nz>

  • needs_better_patch set to 1.
  • needs_tests set to 1.

...and some tests would be nice too.

05/08/07 08:14:41 changed by alex@halogen-dg.com

  • attachment patch.txt added.

trivial patch

05/08/07 08:16:30 changed by alex@halogen-dg.com

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

05/11/07 19:48:02 changed by Jeremy Dunck <jdunck@gmail.com>

  • has_patch set to 1.

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.

05/11/07 19:48:39 changed by Jeremy Dunck <jdunck@gmail.com>

  • attachment djt-2145-one_to_one_field-contribution.diff added.

Contribute to restaurant, not place.

05/24/07 09:05:49 changed by Ilya Semenov <semenov@inetss.com>

+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?).

08/07/07 12:31:27 changed by sgt.hulka@gmail.com

What needs to be done to push this into trunk?

08/15/07 07:20:17 changed by Jos@hyves.nl

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

08/15/07 07:29:33 changed by anonymous

  • needs_better_patch deleted.
  • needs_tests deleted.
  • 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 :)

08/20/07 02:34:57 changed by mtredinnick

  • needs_better_patch set to 1.
  • 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.

(follow-up: ↓ 18 ) 09/15/07 13:31:29 changed by baumer1122

  • status changed from reopened to closed.
  • resolution set to worksforme.

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.

(in reply to: ↑ 17 ) 09/15/07 13:41:32 changed by anonymous

Filed ticket #5497 for limit_choices_to bug

10/09/07 07:50:18 changed by jos@hyves.nl

  • status changed from closed to reopened.
  • resolution deleted.

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

10/09/07 10:44:33 changed 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.

10/10/07 06:10:27 changed 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.

10/10/07 08:24:31 changed 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.)

10/10/07 08:44:09 changed by remco@diji.biz

"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.

10/10/07 13:04:21 changed 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

10/10/07 23:48:24 changed by alex@halogen-dg.com

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

07/19/08 00:43:47 changed by gwilson

  • milestone set to 1.0.

07/24/08 19:24:23 changed by tyson

  • keywords set to OneToOneField ModelInheritance editable parent_link.

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.

07/24/08 19:26:15 changed by tyson

  • attachment 2145-one-to-one-field_editable.diff added.

Patch to allow editing OneToOneFields?

07/24/08 19:50:19 changed 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.

08/05/08 09:58:14 changed by anonymous

  • keywords changed from OneToOneField ModelInheritance editable parent_link to OneToOneField ModelInheritance MultiTableInheritance editable parent_link.

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!!!

08/15/08 13:03:15 changed 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.

08/15/08 14:15:23 changed by brosner

  • status changed from reopened to closed.
  • resolution set to fixed.

(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.

08/15/08 14:17:11 changed by brosner

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


Add/Change #2145 (list_filter not available if model contains OneToOneField)




Change Properties
Action