Change foreign key id list_display reference to display only the id

[Cristiano Coelho]

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.

[Tim Graham]

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.

[Cristiano Coelho]

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.

[krisys]

@cristianocca and @tim, the has_related_field_in_list_display method of ChangeList class in django.contrib.admin.views.main makes use of the django.db.models.options:Options._forward_fields_map which treats the _id attribute in a similar way and the same has been documented there - ​https://github.com/django/django/blob/master/django/db/models/options.py#L469 it specifically talks about the *_id fields as well.

[Cristiano Coelho]

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?

[Tim Graham]

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

[krisys]

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

[Tim Graham]

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

[krisys]

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

[krisys]

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

[Tim Graham]

In f6681393d3f53a67b4e0645e8d02f95579d8ae2d:

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

[Tim Graham <timograham@…>]

In 83120af2:

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

[Tim Graham]

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.

[Jon Dufresne]

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

[Jon Dufresne <jon.dufresne@…>]

In e24c0a2d:

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

Thanks Tim Graham for the regression test.

[Jon Dufresne <jon.dufresne@…>]

In 8b050cf:

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