Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#24146 closed Bug (fixed)

(admin.E116) The value of 'list_filter[0]' refers to 'through__field', which does not refer to a Field.

Reported by: Collin Anderson Owned by: nobody
Component: contrib.admin Version: 1.8alpha1
Severity: Release blocker Keywords:
Cc: pirosb3 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

class PersonAdmin(admin.ModelAdmin):
    list_filter = ['studenthousehold__primary']

class Student(models.Model):
    name = models.CharField(max_length=255)

class Household(models.Model):
    students = models.ManyToManyField(Student, through='StudentHousehold')

class StudentHousehold(models.Model):
    household = models.ForeignKey(Household)
    student = models.ForeignKey(Student)
    primary = models.BooleanField(default=False)

Works fine on 1.7, but on 1.8:

./manage.py check
SystemCheckError: System check identified some issues:

ERRORS:
<class 'app.admin.PersonAdmin'>: (admin.E116) The value of 'list_filter[0]' refers to 'studenthousehold__primary', which does not refer to a Field.

System check identified 1 issue (0 silenced).

Change History (25)

comment:1 by Collin Anderson, 9 years ago

To reproduce:

git clone https://github.com/collinanderson/24146

cd 24146

./manage check

comment:2 by Collin Anderson, 9 years ago

The actual underlying exception is:

  File "django/contrib/admin/checks.py", line 715, in _check_list_filter_item
    get_fields_from_path(model, field)
  File "django/contrib/admin/utils.py", line 473, in get_fields_from_path
    fields.append(parent._meta.get_field(piece))
  File "django/db/models/options.py", line 540, in get_field
    "be available yet." % (self.object_name, field_name)
django.core.exceptions.FieldDoesNotExist: Household has no field named 'studenthousehold'. The app cache isn't ready yet, so if this is a forward field, it won't be available yet.

comment:3 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Collin Anderson, 9 years ago

Here's a simple patch, but it modifies the API.

https://github.com/django/django/pull/3912

comment:5 by Collin Anderson, 9 years ago

Version: master1.8alpha1

comment:6 by Berker Peksag, 9 years ago

Has patch: set

comment:7 by Tim Graham, 9 years ago

Needs tests: set
Version: 1.8alpha1master

Does a test in admin_checks not reproduce the issue because of app loading differences when running the tests?

comment:8 by Collin Anderson, 9 years ago

Exactly, because it's only an issue while apps are loading.

comment:9 by Tim Graham, 9 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In e8171daf0cd7f0e070395cb4c850c17fea32f11d:

Fixed #24146 -- Fixed a missing fields regression in admin checks.

This allows using get_field() early in the app loading process.

Thanks to PirosB3 and Tim Graham.

comment:11 by Carl Meyer, 9 years ago

I'm not convinced this is the right fix. Exposing reverse fields before the app-cache is ready is a recipe for unpredictable behavior, which is exactly what app-loading and the meta-refactor were supposed to fix. Why do admin checks run before the app cache is ready? That seems wrong - can't we fix that instead?

comment:12 by Collin Anderson, 9 years ago

The admin checks run in ready(), and apps.ready isn't True until all ready()s are done.

https://github.com/django/django/blob/570912a97d5051fa3aeacd9d16c3be9afcf92198/django/apps/registry.py#L111-L114

comment:13 by Carl Meyer, 9 years ago

It seems we already have two stages of app-registry "readyness" -- there's models_ready, which is set to True before the app-config ready() methods run, and then there's ready, which isn't set until afterwards. It seems to me that it's really models_ready which should be the basis for the meta API deciding that the model graph is ready for use; in general, app-config ready() methods should be able to introspect models in a deterministic way.

So I don't think we should introduce the possibility of non-deterministic introspection into the core of the meta API, as this commit does. Instead, we should switch the meta API to require models_ready rather than require ready before it caches the full model graph.

comment:14 by Collin Anderson, 9 years ago

We're kind of repeating the discussion on the PR. :)
https://github.com/django/django/pull/3912

In my head I though Daniel was opposed to it, but apparently on the PR he said "I think models_ready is a good place.".

comment:15 by Collin Anderson, 9 years ago

Anyway, I'm not attached to any particular solution, as long as it works :)

comment:16 by Carl Meyer, 9 years ago

I don't have time in the next couple days, but towards the end of this week I can put together an alternative PR that maintains deterministic behavior but uses models_ready instead of ready. I think I'd feel better if the existing commit were rolled back sooner so we don't get anything else that depends on the non-deterministic early calling of get_field(), but I can also just roll it back in my alternate PR.

comment:17 by Tim Graham <timograham@…>, 9 years ago

In 0e489c19f1554ecfd9825daacfbac73be8ce723e:

Reverted "Fixed #24146 -- Fixed a missing fields regression in admin checks."

This reverts commit e8171daf0cd7f0e070395cb4c850c17fea32f11d.

A new solution is forthcoming.

comment:18 by Tim Graham <timograham@…>, 9 years ago

In 92d5bedc56155e4c91c7644200415b3e42cdd31f:

[1.8.x] Reverted "Fixed #24146 -- Fixed a missing fields regression in admin checks."

This reverts commit e8171daf0cd7f0e070395cb4c850c17fea32f11d.

A new solution is forthcoming.

Backport of 0e489c19f1554ecfd9825daacfbac73be8ce723e from master

comment:19 by Tim Graham, 9 years ago

Has patch: unset
Triage Stage: Ready for checkinAccepted
Version: master1.8alpha1

comment:20 by Tim Graham, 9 years ago

Resolution: fixed
Status: closednew

comment:21 by pirosb3, 9 years ago

Cc: pirosb3 added

comment:22 by pirosb3, 9 years ago

Hi all,

Yes, I am happy for the check to go on models_ready . This should resolve the current issue and also another one carljm reported earlier. We should make sure that reverse models are always accessible on anything that is public API, so this includes any hooks or ModelAdmin registrations. Can anyone think of other similar parts that should be tested?

If we are going in this direction (given that what I said above is correct), I have made a PR with a few changes: https://github.com/django/django/pull/4053. After we assure Jenkins passes, I will add a few other tests admin-specific.

comment:23 by pirosb3, 9 years ago

Has patch: set
Needs tests: set

comment:24 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 19188826b4aa989475418f1ea9bf8631b04da1e8:

Fixed #24146 -- Allowed model._meta.get_field() to be used after apps.models_ready

comment:25 by Tim Graham <timograham@…>, 9 years ago

In fdcc9c47d51289576ce17849900c96c161273037:

[1.8.x] Fixed #24146 -- Allowed model._meta.get_field() to be used after apps.models_ready

Backport of 19188826b4aa989475418f1ea9bf8631b04da1e8 from master

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