Changes between Initial Version and Version 1 of Ticket #37230, comment 13
- Timestamp:
- Aug 11, 2026, 6:26:52 AM (4 days ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
Ticket #37230, comment 13
initial v1 1 1 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. 2 2 3 ##The patch is a relocation, not a fix3 The patch is a relocation, not a fix 4 4 5 5 The diff moves the unconditional field lookup from `utils.py::lookup_field()` into `admin_list.py::items_for_result()` verbatim: 6 6 7 ```diff7 {{{#!diff 8 8 --- a/django/contrib/admin/utils.py 9 9 +++ b/django/contrib/admin/utils.py … … 16 16 - f = get_fields_from_path(opts.model, name)[-1] 17 17 value = attr 18 ``` 19 ```diff 18 20 19 --- a/django/contrib/admin/templatetags/admin_list.py 21 20 +++ b/django/contrib/admin/templatetags/admin_list.py … … 31 30 + pass # e.g. __str__ 32 31 if f is None or f.auto_created: 33 ``` 32 }}} 34 33 35 34 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: 36 35 37 ```python36 {{{#!python 38 37 if isinstance(f.remote_field, models.ManyToOneRel): 39 38 field_val = getattr(result, f.name) 40 ``` 39 }}} 41 40 42 41 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. … … 44 43 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: 45 44 46 ```python45 {{{#!python 47 46 class Publisher(models.Model): 48 47 name = models.CharField(max_length=100) … … 59 58 class AuthorAdmin(admin.ModelAdmin): 60 59 list_display = ["name", "book__publisher"] 61 ``` 60 }}} 62 61 63 62 Visiting the `Author` changelist with at least one row still raises: 64 63 65 ``` 64 {{{ 66 65 AttributeError: 'Author' object has no attribute 'publisher' 67 ``` 66 }}} 68 67 69 68 ## Why the regression test didn't catch this … … 71 70 The test added in the patch, `UtilsTests.test_values_from_lookup_field` (`tests/admin_utils/tests.py`), only adds this case: 72 71 73 ```python72 {{{#!python 74 73 ("site__parent", None), 75 ``` 74 }}} 76 75 77 76 and asserts it via: 78 77 79 ```python78 {{{#!python 80 79 field, attr, resolved_value = lookup_field(name, article, mock_admin) 81 80 if field is not None: 82 81 resolved_value = display_for_field(resolved_value, field, self.empty_value) 83 82 self.assertEqual(value, resolved_value) 84 ``` 83 }}} 85 84 86 85 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. … … 88 87 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. 89 88 90 ##Suggested test89 Suggested test 91 90 92 91 Something along these lines, exercising the actual admin list rendering rather than the isolated helper: 93 92 94 ```python93 {{{#!python 95 94 def test_list_display_second_degree_relation_crash(self): 96 95 """ … … 109 108 # Should not raise AttributeError. 110 109 list(items_for_result(cl, article, None)) 111 ``` 110 }}} 112 111 113 112 ## Suggested fix … … 115 114 Actually apply the guard the original report proposed: 116 115 117 ```python116 {{{#!python 118 117 if f is None and isinstance(field_name, str) and LOOKUP_SEP in field_name: 119 118 try: … … 123 122 except FieldDoesNotExist: 124 123 pass # e.g. __str__ 125 ``` 124 }}} 126 125 127 126 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.