Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#26524 closed Cleanup/optimization (fixed)

Change foreign key id list_display reference to display only the id

Reported by: Cristiano Coelho Owned by: nobody
Component: contrib.admin Version: dev
Severity: Release blocker Keywords: admin list_display changelist
Cc: Cristiano Coelho, krishna.bmsce@…, zachborboa@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tobin Brown)

Given these two models.

class A(models.Model):
    name = models.CharField(max_length=100)

    def __unicode__(self):
        return self.name

class B(models.Model):
    name = models.CharField(max_length=100)
    fk = models.ForeignKey(A)
        
    def __unicode__(self):
        return self.name

And these model admin

class AAdmin(admin.ModelAdmin):
    list_display = ('id','name')
admin.site.register(A, AAdmin)

class BAdmin(admin.ModelAdmin):
    list_display = ('id','name','fk_id','fk')
admin.site.register(B, BAdmin)

As you see, for BAdmin I'm trying to display the actual id of the foreign key, so I prevent an additional unwanted join (or worse, one query per related object if no select related is added). However, the admin page refuses to display the id and instead renders the whole object (calling the unicode method) which causes the additional join I don't want.
In order to demostrate it I also added the 'fk' relation which works as expected

The change list result is then

1	test B	test A	test A

where it should be

1	test B	1	test A

In order to work around it, I can define a callable that just returns the id property, but that's terrible because I lose any sort option on it plus I need to write quite a few more lines.

Attachments (1)

26524-regress.diff (1.4 KB ) - added by Tim Graham 7 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Tim Graham, 8 years ago

Summary: Django Admin list_display - Unable to display foreign key idChange foreign key id list_display reference to display only the id
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Seems reasonable, though is backwards-incompatible -- hopefully not too many people will be affected by the change but I'd mention it in the release notes.

comment:2 by Cristiano Coelho, 8 years ago

Is there any specific reason that using 'fk_id' and just 'fk' causes both to actually fetch the object? I'm expecting that if I access fk_id I am actually accessing the attribute and not the relation.

It is the same case when working with instances:

b = B.objects.first()
print b.fk_id --> prints the id/value
print b.fk --> fetchs the relation and call the actual object __unicode__

So for me this working different when using the ModelAdmin and display_list looks like a bug rather than a cleanup, you have two properties performing the exact same action.

comment:3 by krisys, 8 years ago

I don't think this is specific to admin. @cristianocca and @tim, this behavior is documented in the following method - django.db.models.options:Options._forward_fields_map - https://github.com/django/django/blob/master/django/db/models/options.py#L469 which specifically talks about the *_id fields as well.

Version 1, edited 8 years ago by krisys (previous) (next) (diff)

comment:4 by Cristiano Coelho, 8 years ago

So actually changing this could break a few things? Any other work around that could let you use the *_id field but still allow for sorting of that column, which using a custom callable prevents?

comment:5 by Tim Graham, 8 years ago

Not necessarily, changing it at the admin list_display level might be feasible.

comment:6 by krisys, 8 years ago

Cc: krishna.bmsce@… added

comment:7 by krisys, 8 years ago

@timgraham, I have submitted a pull request for the same https://github.com/django/django/pull/6497
Will be happy to know if there are any better ways of doing this.

comment:8 by Tim Graham, 8 years ago

Has patch: set

Please check "Has patch" so the ticket appears in the review queue.

comment:9 by krisys, 8 years ago

@timgraham, Thanks. Will do that from now onwards.

comment:10 by Zach Borboa, 8 years ago

Cc: zachborboa@… added

comment:11 by Tobin Brown, 8 years ago

Description: modified (diff)

comment:12 by krisys, 8 years ago

@timgraham, Any update on this? Will be glad to work on anything that can make the review and merge process easy w.r.t this patch.

comment:13 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed

In f6681393d3f53a67b4e0645e8d02f95579d8ae2d:

Fixed #26524 -- Made a foreign key id reference in ModelAdmin.list_display display the id.

comment:14 by Tim Graham <timograham@…>, 8 years ago

In 83120af2:

Refs #26524 -- Fixed an error in 1.11 release notes.

comment:15 by Tim Graham, 7 years ago

Has patch: unset
Severity: NormalRelease blocker
Version: 1.9master

This caused a regression in using a ManyToManyField in ModelAdmin.readonly_fields. I'm attaching a regression test that passes on the stable/1.10.x branch. The {% include %} in the admin template started silently failing in the commit for this ticket and then in 331ca5391eb64cbac3a001209257beb93522d587, that silent failure become an exception because of the deprecation warning. Fixing this will also fix the two selenium test failures that use the Pizza admin.

comment:16 by Tim Graham, 7 years ago

Resolution: fixed
Status: closednew

by Tim Graham, 7 years ago

Attachment: 26524-regress.diff added

comment:17 by Jon Dufresne, 7 years ago

Has patch: set

Potential fix

If I understand the surrounding code and intentions correctly, I think the PR fixes the regression by changing the condition from:

    # Avoid coercing <FK>_id fields to FK
    if field.is_relation and hasattr(field, 'attname') and field.attname == name:
        raise FieldIsAForeignKeyColumnName()

To

    # Avoid coercing <FK>_id fields to FK
    if field.is_relation and not field.many_to_many and hasattr(field, 'attname') and field.attname == name:
        raise FieldIsAForeignKeyColumnName()

But it would help if someone with a deeper understanding of the original change, admin, and relationship fields had a look to make sure I'm not introducing a different edge case crash. Thanks for any feedback.

comment:18 by Tim Graham, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:19 by Jon Dufresne <jon.dufresne@…>, 7 years ago

Resolution: fixed
Status: newclosed

In e24c0a2d:

Fixed #26524 -- Fixed crash in admin change view when displaying many to many forward refs.

Thanks Tim Graham for the regression test.

comment:20 by Jon Dufresne <jon.dufresne@…>, 7 years ago

In 8b050cf:

Refs #26524 -- Added a test for a <OneToOneField>_id reference in ModelAdmin.list_display.

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