Opened 3 weeks ago

Closed 5 days ago

Last modified 4 days ago

#37230 closed Bug (fixed)

ModelAdmin.list_display: AttributeError when a __ lookup path ends in a relation field (regression from #36926)

Reported by: RobKuipers Owned by: Zubair Hassan
Component: contrib.admin Version: 6.1
Severity: Release blocker Keywords:
Cc: RobKuipers 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

Since #36926, list_display entries using __ lookup syntax to traverse related fields (documented at https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_display) raise AttributeError when the *final* segment of the path is itself a relation field (ForeignKey/OneToOneField), rather than a scalar field.

This worked correctly before #36926 and is not limited to any unusual setup — any related__fk_field entry regresses.

Minimal reproduction

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

class Book(models.Model):
    title = models.CharField(max_length=100)
    publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE)

class Author(models.Model):
    name = models.CharField(max_length=100)
    book = models.ForeignKey(Book, on_delete=models.CASCADE)

@admin.register(Author)
class AuthorAdmin(admin.ModelAdmin):
    list_display = ["name", "book__publisher"]

Visiting the Author changelist with at least one row raises:

AttributeError: 'Author' object has no attribute 'publisher'

Root cause

#36926 added this to lookup_field() in django/contrib/admin/utils.py,
specifically to let display_for_field() recognize BooleanFields reached
via a __ path so it can render the boolean icon:

                    # The final field is needed for displaying boolean icons.
                    if LOOKUP_SEP in name:
                        f = get_fields_from_path(opts.model, name)[-1]

This sets f unconditionally whenever the path contains LOOKUP_SEP, not only when the terminal field is a BooleanField. Previously, f stayed
None for this whole branch (the "method, property, related field, or callable" fallback), and items_for_result() in django/contrib/admin/templatetags/admin_list.py used the already correctly-traversed value via display_for_value().

Now that f is non-None, items_for_result() takes a different branch that assumes f is a field belonging to result directly:

            else:
                if isinstance(f.remote_field, models.ManyToOneRel):
                    field_val = getattr(result, f.name)

That assumption holds for the normal case (list_display = ["publisher"] where publisher really is a field on the model being rendered), but not for the __-traversal case, where f is a field on the *related* model several hops down the path — result (an Author) has no publisher attribute; result.book does.

Suggested fix direction

Scope the new f assignment to its stated purpose — only set it when the terminal field is actually a BooleanField — rather than for every LOOKUP_SEP-containing path:

                    if LOOKUP_SEP in name:
                        final_field = get_fields_from_path(opts.model, name)[-1]
                        if isinstance(final_field, models.BooleanField):
                            f = final_field

This preserves the boolean-icon feature #36926 added while restoring the pre-6.1 (and documented) behavior for __ paths ending in any other field
type, including relations.

I have not attempted a full patch/tests — that's a little out of my league I'm afraid.

Change History (15)

comment:1 by David Smith, 3 weeks ago

Triage Stage: UnreviewedAccepted

I was able to replicate this issue and saw that the behaviour changed in 4b2b4bf0ac2707dc9c4d51cabfa72168eaea95fe. Given this is a new change for 6.1 the triage state of release blocker seems right to me.

comment:2 by Zubair Hassan, 3 weeks ago

Owner: set to Zubair Hassan
Status: newassigned

comment:4 by Clifford Gama, 3 weeks ago

Needs tests: set

comment:5 by Clifford Gama, 3 weeks ago

Needs documentation: set

comment:6 by Clifford Gama, 3 weeks ago

Needs documentation: unset

Patch is missing regression tests. (No docs changes required, please ignore the changes to that ticket flag.)

comment:7 by Zubair Hassan, 3 weeks ago

Added regression tests.

comment:8 by Sarah Boyce, 3 weeks ago

Version: dev6.1

comment:9 by Jacob Walls, 3 weeks ago

Needs tests: unset
Patch needs improvement: set

comment:10 by Jacob Walls, 3 weeks ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:11 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 92470ad3:

Fixed #37230 -- Fixed a crash for second-degree relations in ModelAdmin.list_display.

Thanks Rob Kuipers for the report.

Regression in 4b2b4bf0ac2707dc9c4d51cabfa72168eaea95fe.

Co-authored-by: Jacob Walls <jacobtylerwalls@…>

comment:12 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

In 724d8760:

[6.1.x] Fixed #37230 -- Fixed a crash for second-degree relations in ModelAdmin.list_display.

Thanks Rob Kuipers for the report.

Regression in 4b2b4bf0ac2707dc9c4d51cabfa72168eaea95fe.

Co-authored-by: Jacob Walls <jacobtylerwalls@…>

Backport of 92470ad3742524902b29769d2c822dbe791630db from main.

comment:13 by RobKuipers, 7 days ago

Resolution: fixed
Status: closednew

Reopening — the merged fix (92470ad3 on main, backport 724d8760 on stable/6.1.x) doesn't actually resolve this, and it shipped in the 6.1 release.

## The patch is a relocation, not a fix

The diff moves the unconditional field lookup from utils.py::lookup_field() into admin_list.py::items_for_result() verbatim:

`diff
--- a/django/contrib/admin/utils.py
+++ b/django/contrib/admin/utils.py
@@ -314,9 +314,6 @@ def lookup_field(name, obj, model_admin=None):

attr = getattr(attr, part, sentinel)
if attr is sentinel:

return None, None, None

  • # The final field is needed for displaying boolean icons.
  • if LOOKUP_SEP in name:
  • f = get_fields_from_path(opts.model, name)[-1]

value = attr

`
`diff
--- a/django/contrib/admin/templatetags/admin_list.py
+++ b/django/contrib/admin/templatetags/admin_list.py
@@ -226,6 +226,12 @@ def link_in_col(is_first, field_name, cl):

empty_value_display = getattr(

attr, "empty_value_display", empty_value_display

)

+ # Find boolean fields on relations.
+ if f is None and isinstance(field_name, str) and LOOKUP_SEP in field_name:
+ try:
+ f = get_fields_from_path(cl.model, field_name)[-1]
+ except FieldDoesNotExist:
+ pass # e.g. str

if f is None or f.auto_created:

`

The isinstance(final_field, models.BooleanField) guard proposed in the original report was never added on either side. f is still set for *any* __-path, boolean-terminated or not, so items_for_result() still reaches:

`python
if isinstance(f.remote_field, models.ManyToOneRel):

field_val = getattr(result, f.name)

`

for a multi-hop path ending in a FK/O2O, which still raises AttributeError exactly as in the original report, because f.name (e.g. "publisher") is a field on the *related* model several hops down the path, not on result itself.

Confirmed against the actual released artifact, not a local/cached install: downloaded django-6.1-py3-none-any.whl fresh from PyPI, verified its sha256 (6c132cd980c9392b06807d4ca52d72530d631dc65a85d9dacede00a780cefbbe) matches the published metadata, and unzipped it — admin_list.py lines 230–234 match the "fixed" diff above exactly. Reproduction:

`python
class Publisher(models.Model):

name = models.CharField(max_length=100)

class Book(models.Model):

title = models.CharField(max_length=100)
publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE)

class Author(models.Model):

name = models.CharField(max_length=100)
book = models.ForeignKey(Book, on_delete=models.CASCADE)

@admin.register(Author)
class AuthorAdmin(admin.ModelAdmin):

list_display = ["name", "bookpublisher"]

`

Visiting the Author changelist with at least one row still raises:

`
AttributeError: 'Author' object has no attribute 'publisher'
`

## Why the regression test didn't catch this

The test added in the patch, UtilsTests.test_values_from_lookup_field (tests/admin_utils/tests.py), only adds this case:

`python
("siteparent", None),
`

and asserts it via:

`python
field, attr, resolved_value = lookup_field(name, article, mock_admin)
if field is not None:

resolved_value = display_for_field(resolved_value, field, self.empty_value)

self.assertEqual(value, resolved_value)
`

This calls lookup_field() directly — it never goes through admin_list.items_for_result(), which is where the actual crash originates. Since the patch *removed* the f-setting logic from lookup_field() (rather than fixing it), lookup_field('site__parent', article, mock_admin) now correctly returns f=None, and the traversed value resolves to None because site_obj.parent (an unsaved FK) defaults to None. The assertion passes trivially — it's validating a function that was never broken. The function that's still broken (items_for_result()) is never exercised by this test at all, so the regression test doesn't actually cover the regression.

Concretely, even the field chosen for the test (site__parent, always None in the fixture) couldn't have caught this: the crash isn't conditional on the FK's value being null vs. set — getattr(result, f.name) fails whenever result simply lacks an attribute named f.name, which is true here regardless of what site.parent holds. The test would need to render an actual changelist (or call items_for_result() directly) to reach the failing line.

## Suggested test

Something along these lines, exercising the actual admin list rendering rather than the isolated helper:

`python
def test_list_display_second_degree_relation_crash(self):

"""
Regression test for #37230: a list_display entry using a __ lookup
that terminates in a non-boolean relation field must not crash.
"""
site_obj = Site.objects.create(domain="example.com")
article = Article.objects.create(site=site_obj, title="Title", created=datetime.min)

class ArticleAdmin(admin.ModelAdmin):

list_display = ["title", "siteparent"]

ma = ArticleAdmin(Article, admin.site)
request = self._mocked_authenticated_request("/", self.superuser)
cl = ma.get_changelist_instance(request)
# Should not raise AttributeError.
list(items_for_result(cl, article, None))

`

## Suggested fix

Actually apply the guard the original report proposed:

`python
if f is None and isinstance(field_name, str) and LOOKUP_SEP in field_name:

try:

final_field = get_fields_from_path(cl.model, field_name)[-1]
if isinstance(final_field, models.BooleanField):

f = final_field

except FieldDoesNotExist:

pass # e.g. str

`

This preserves the boolean-icon feature the original patch (regression source, 4b2b4bf0) intended, while restoring correct behavior for __ paths ending in any other field type, including relations.

Version 0, edited 7 days ago by RobKuipers (next)

comment:14 by Sarah Boyce, 5 days ago

Resolution: fixed
Status: newclosed

Please don't reopen closed tickets. You should create a new ticket if you want to report a regression.

in reply to:  14 comment:15 by Jacob Walls, 5 days ago

Replying to Sarah Boyce:

Please don't reopen closed tickets. You should create a new ticket if you want to report a regression.

I went ahead and started investigating. I opened #37270 and a PR.

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