Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  closed
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by goblinJoel):

 (I found this thread after already finding the commit that caused this
 problem for me, so this is copied/edited from
 
[https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1#commitcomment-34636977
 my comment there])

 While upgrading from Django 1.11 to 2.2, I found this change causes one of
 our custom field types to fail system checks when used in an admin's
 list_display. The dev who made the field had overridden
 contribute_to_class() to assign a descriptor class that raises an
 AttributeError on __get__() if no instance is supplied, making the field
 attribute only accessible from instances and not from the class itself.

 Previously, this still worked, as model._meta.get_field(item) returned
 true. Now, hasattr() must also be true.

 I'm pretty sure I can change our __get__() to just return the descriptor
 in that case, as ImageField's descriptor does
 
[https://github.com/django/django/commit/9f6b704769ba5bc0daafc25340d3dc28b18d8fb1
 from 1.10 on], but I thought I'd note this in case there was some reason
 the behavior I described should be supported.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.7e05ca6ef682541675fd8c99c15ab0fd%40djangoproject.com.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  closed
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak ):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"ed668796f6c7e59ca3f35e9d87d940e043a3eff3" ed668796]:
 {{{
 #!CommitTicketReference repository=""
 revision="ed668796f6c7e59ca3f35e9d87d940e043a3eff3"
 Fixed #30543 -- Fixed checks of ModelAdmin.list_display for fields
 accessible only via instance.

 Co-Authored-By: Andrew Simons 
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.0eb0e09863b15e9947190cbe3b44c9a0%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  assigned
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Hasan Ramezani):

 * needs_tests:  1 => 0


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.15017e2e95f500c07fc5d3acd969e2f4%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  assigned
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by felixxm):

 * needs_tests:  0 => 1


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.cbc32952dc775c039f967f5accb1db10%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  assigned
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by ajcsimons):

 * has_patch:  0 => 1


Comment:

 My proposed changes as pull request:
 [https://github.com/django/django/pull/11444/ PR]

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.ff9712f51e1d4cf1e1d153536a82c149%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
-+-
 Reporter:  ajcsimons|Owner:  ajcsimons
 Type:  Bug  |   Status:  assigned
Component:  Core (System |  Version:  master
  checks)|
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by ajcsimons):

 * status:  new => assigned
 * owner:  nobody => ajcsimons


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.6699e2c7f7e397f5467af8d5b9ac604c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+

Comment (by ajcsimons):

 I think there is a potential simplification of my solution: if get_field
 raises the FieldDoesNotExist exception then I suspect getattr(obj.model,
 item) might always fail too (so that test could be omitted and go straight
 to returning the E108 error), but I'm not sure because the inner workings
 of get_field are rather complicated.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.f16f1644c0f6ec43e0c9591477044e44%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+
Description changed by ajcsimons:

Old description:

> As part of startup django validates the ModelAdmin's list_display
> list/tuple for correctness
> (django.admin.contrib.checks._check_list_display). Having upgraded django
> from 2.07 to 2.2.1 I found that a ModelAdmin with a list display that
> used to pass the checks and work fine in admin now fails validation,
> preventing django from starting. A PositionField from the django-
> positions library triggers this bug, explanation why follows.
>
> {{{
> from django.db import models
> from position.Fields import PositionField
>
> class Thing(models.Model)
>   number = models.IntegerField(default=0)
>   order = PositionField()
> }}}
>
> {{{
> from django.contrib import admin
> from .models import Thing
>
> @admin.register(Thing)
> class ThingAdmin(admin.ModelAdmin)
>   list_display = ['name', 'order']
> }}}
>
> Under 2.2.1 this raises an incorrect admin.E108 message saying "The value
> of list_display[1] refers to 'order' which is not a callable...".
> Under 2.0.7 django starts up successfully.
> If you change 'name' to 'no_name' or 'order' to 'no_order' then the
> validation correctly complains about those.
>
> The reason for this bug is commit
> https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1
> which was proposed and accepted as a fix for bug
> https://code.djangoproject.com/ticket/28490. The problem is while it
> fixed that bug it broke the functionality of _check_list_display_item in
> other cases. The rationale for that change was that after
> field=getattr(model, item) field could be None if item was a descriptor
> returning None, but subsequent code incorrectly interpreted field being
> None as meaning getattr raised an AttributeError. As this was done
> **after** trying field = model._meta.get_field(item) and that failing
> that meant the validation error should be returned. However, after the
> above change if hasattr(model, item) is false then we no longer even try
> field = model._meta.get_field(item) before returning an error. The reason
> hasattr(model, item) is false in the case of a PositionField is its
> __get__ method throws an exception if called on an instance of the
> PositionField class on the Thing model class, rather than a Thing
> instance.
>
> For clarity, here are the various logical tests that
> _check_list_display_item needs to deal with and the behaviour before the
> above change, after it, and the correct behaviour (which my suggested
> patch exhibits). Note this is assuming the first 2 tests callable(item)
> and hasattr(obj, item) are both false (corresponding to item is an actual
> function/lambda rather than string or an attribute of ThingAdmin).
>
> * hasattr(model, item) returns True or  False (which is the same as
> seeing if getattr(model, item) raises AttributeError)
> * model._meta.get_field(item) returns a field or raises FieldDoesNotExist
> Get a field from somewhere, could either be from getattr(model,item) if
> hasattr was True or from get_field.
> * Is that field an instance of ManyToManyField?
> * Is that field None? (True in case of bug 28490)
>
> ||= hasattr  =||= get_field =||= field is None? =||= field ManyToMany?
> =||= 2.0 returns =||= 2.2 returns =||= Correct behaviour =||= Comments
> =||
> || True ||  ok  || False || False || [] || [] || [] || - ||
> || True ||  ok  || False || True || E109 || E109 || E109 || - ||
> || True ||  ok  || True || False || E108 || [] || [] || good bit of 28490
> fix, 2.0 was wrong ||
> || True ||  raises || False || False || [] || [] || [] || - ||
> || True ||  raises || False || True || E109 || [] || E109 || Another bug
> introduced by 28490 fix, fails to check if ManyToMany in get_field raise
> case ||
> || True ||  raises || True || False || E108 || [] || [] || good bit of
> 28490 fix, 2.0 was wrong ||
> || False ||  ok || False || False || [] || E108 || [] || bad bit of 28490
> fix, bug hit with PositionField ||
> || False ||  ok || False || True || [] || E108 || E109 || both 2.0 and
> 2.2 wrong ||
> || False ||  ok || True || False || [] || E108 || [] || bad 28490 fix ||
> || False ||  raises || False || False || E108 || E108 || E108 || - ||
> || False ||  raises |

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+

Old description:

> #28490 caused a regression in 47016adbf54b54143d4cf052eeb29fc72d27e6b1,
> i.e.
>
> 1. if hasattr(obj.model, item) returns false then we go straight to the
> else clause which returns the error,
> 2. whereas before the else clause did another check for
> model._meta.get_field(item) and would only return the error if that
> raised a FieldDoesNotExist exception
> 3. So how is it that hasattr(model, item) can return false, but
> model._meta.get_field(item) will return something meaning the check
> should not return an error?
> 4. Answer: the field is a special one which is only valid to access on
> instances of the model (whose ModelAdmin we are verifying) and not the
> model class itself. An example of this is the PositionField from the
> django-positions library (which inherits from djangos
> models.IntegerField):
> {{{
> def __get__(self, instance, owner):
> if instance is None:
> raise AttributeError("%s must be accessed via instance." %
> self.name)
> current, updated = getattr(instance, self.get_cache_name())
> return current if updated is None else updated
> }}}
> 5. So the simplification of the hasattr branch was valid, but the removal
> of the else branch to no longer check get_field doesn't throw before
> returning an error was not a refactor but a functionality altering change
> which makes this method return errors in cases where it used not to.

New description:

 As part of startup django validates the ModelAdmin's list_display
 list/tuple for correctness
 (django.admin.contrib.checks._check_list_display). Having upgraded django
 from 2.07 to 2.2.1 I found that a ModelAdmin with a list display that used
 to pass the checks and work fine in admin now fails validation, preventing
 django from starting. A PositionField from the django-positions library
 triggers this bug, explanation why follows.

 {{{
 from django.db import models
 from position.Fields import PositionField

 class Thing(models.Model)
   number = models.IntegerField(default=0)
   order = PositionField()
 }}}

 {{{
 from django.contrib import admin
 from .models import Thing

 @admin.register(Thing)
 class ThingAdmin(admin.ModelAdmin)
   list_display = ['name', 'order']
 }}}

 Under 2.2.1 this raises an incorrect admin.E108 message saying "The value
 of list_display[1] refers to 'order' which is not a callable...".
 Under 2.0.7 django starts up successfully.
 If you change 'name' to 'no_name' or 'order' to 'no_order' then the
 validation correctly complains about those.

 The reason for this bug is commit
 
https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1
 which was proposed and accepted as a fix for bug
 https://code.djangoproject.com/ticket/28490. The problem is while it fixed
 that bug it broke the functionality of _check_list_display_item in other
 cases. The rationale for that change was that after field=getattr(model,
 item) field could be None if item was a descriptor returning None, but
 subsequent code incorrectly interpreted field being None as meaning
 getattr raised an AttributeError. As this was done **after** trying field
 = model._meta.get_field(item) and that failing that meant the validation
 error should be returned. However, after the above change if
 hasattr(model, item) is false then we no longer even try field =
 model._meta.get_field(item) before returning an error. The reason
 hasattr(model, item) is false in the case of a PositionField is its
 __get__ method throws an exception if called on an instance of the
 PositionField class on the Thing model class, rather than a Thing
 instance.

 For clarity, here are the various logical tests that
 _check_list_display_item needs to deal with and the behaviour before the
 above change, after it, and the correct behaviour (which my suggested
 patch exhibits). Note this is assuming the first 2 tests callable(item)
 and hasattr(obj, item) are both false (corresponding to item is an actual
 function/lambda rather than string or an attribute of ThingAdmin).

 * hasattr(model, item) returns True or  False (which is the same as seeing
 if getattr(model, item) raises AttributeError)
 * model._meta.get_field(ite

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+

Comment (by ajcsimons):

 Hi felixxm, I also just made a ticket #30545 with more details. Working
 through all the logical combinations I think both the old code and new
 code have other bugs and I've posted a suggested fix there.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.533c70daf4256db96fab33e8268605c0%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+

Comment (by felixxm):

 Fix is quite simple but a regression test can be tricky.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.a40d4df2e7dc135967b70f3e0f4670ad%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
--+
 Reporter:  ajcsimons |Owner:  nobody
 Type:  Bug   |   Status:  new
Component:  Core (System checks)  |  Version:  master
 Severity:  Normal|   Resolution:
 Keywords:| Triage Stage:  Accepted
Has patch:  0 |  Needs documentation:  0
  Needs tests:  0 |  Patch needs improvement:  0
Easy pickings:  0 |UI/UX:  0
--+
Changes (by felixxm):

 * Attachment "30543.diff" added.

 fix

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.c9200a748f806e2d9cc39d356fb66977%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

[Django] #30543: admin.E108 is raised on fields accessible only via instance.

[Django]

#30543: admin.E108 is raised on fields accessible only via instance.
+
   Reporter:  ajcsimons |  Owner:  nobody
   Type:  Bug   | Status:  new
  Component:  Core (System checks)  |Version:  master
   Severity:  Normal|   Keywords:
   Triage Stage:  Accepted  |  Has patch:  0
Needs documentation:  0 |Needs tests:  0
Patch needs improvement:  0 |  Easy pickings:  0
  UI/UX:  0 |
+
 #28490 caused a regression in 47016adbf54b54143d4cf052eeb29fc72d27e6b1,
 i.e.

 1. if hasattr(obj.model, item) returns false then we go straight to the
 else clause which returns the error,
 2. whereas before the else clause did another check for
 model._meta.get_field(item) and would only return the error if that raised
 a FieldDoesNotExist exception
 3. So how is it that hasattr(model, item) can return false, but
 model._meta.get_field(item) will return something meaning the check should
 not return an error?
 4. Answer: the field is a special one which is only valid to access on
 instances of the model (whose ModelAdmin we are verifying) and not the
 model class itself. An example of this is the PositionField from the
 django-positions library (which inherits from djangos
 models.IntegerField):
 {{{
 def __get__(self, instance, owner):
 if instance is None:
 raise AttributeError("%s must be accessed via instance." %
 self.name)
 current, updated = getattr(instance, self.get_cache_name())
 return current if updated is None else updated
 }}}
 5. So the simplification of the hasattr branch was valid, but the removal
 of the else branch to no longer check get_field doesn't throw before
 returning an error was not a refactor but a functionality altering change
 which makes this method return errors in cases where it used not to.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/052.4d8bdef7eefa247c816e2c57b6253074%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.