Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  admin, descriptor,   | Triage Stage:  Accepted
  system, checks, framework, |
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by felixxm):

 Thanks for the report, I added a separate ticket #30543 for this issue.

-- 
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.f4860a015e7c5a5b5cb6c6a6960ce46f%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  admin, descriptor,   | Triage Stage:  Accepted
  system, checks, framework, |
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by ajcsimons):

 Unfortuantely this fix causes other bugs:
 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
 }}}

-- 
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.8b4d46cc4197dfcb962245f369bf0207%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  admin, descriptor,   | Triage Stage:  Accepted
  system, checks, framework, |
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Hunter Richards):

 Thanks for the help and review, Tim!

-- 
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.d419134d595ee573fac274ed76e1e178%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  admin, descriptor,   | Triage Stage:  Accepted
  system, checks, framework, |
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Tim Graham ):

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


Comment:

 In [changeset:"47016adbf54b54143d4cf052eeb29fc72d27e6b1" 47016adb]:
 {{{
 #!CommitTicketReference repository=""
 revision="47016adbf54b54143d4cf052eeb29fc72d27e6b1"
 Fixed #28490 -- Removed unused code in admin.E108 check.
 }}}

-- 
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.a326098b7237454352d733a185597ece%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  admin, descriptor,   | Triage Stage:  Accepted
  system, checks, framework, |
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Tim Graham):

 * stage:  Unreviewed => Accepted


Comment:

 I think it would be fine to do the simplification but I'd omit the test
 for a broken descriptor that returns `None`.

-- 
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.1d8ef90ceee479bc5aa8195fbef9369a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  admin, descriptor,   | Triage Stage:
  system, checks, framework, |  Unreviewed
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Hunter Richards):

 Thanks for the reply, Tim!

 I can't think of a use case for a Descriptor that returns `None` that
 triggers the error reported here, but the Descriptor API makes it very
 easy to shoot yourself in the foot by writing one, and the Python
 Descriptor documentation includes many examples that would trigger this
 error, along with no word on having to manually handle the `if obj is
 None` case (although the `@property` example is correctly written):

 https://docs.python.org/3/howto/descriptor.html

 We initially discovered this bug due to such an improperly written
 Descriptor, and it took us a long time to track down the reason due to the
 triggering of the error message being so deep.  We wanted to save other
 Djangonauts that time in the future!

 Might I also point out that this PR includes a simplification of the
 affected Admin Check code, including the removal of a known code
 duplication, regardless of the prevalence of `None`-returning Descriptors.

-- 
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.b76a7cac217a163c261dc758b85602ca%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  admin, descriptor,   | Triage Stage:
  system, checks, framework, |  Unreviewed
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham):

 What's the use case for a descriptor returning `None`?

-- 
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.110f82df3f8e3be1960863413ccb9116%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
 Reporter:  Hunter Richards  |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  admin, descriptor,   | Triage Stage:
  system, checks, framework, |  Unreviewed
  validation |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Hunter Richards):

 * Attachment "modeladmin_none_descriptor_error.diff" added.


-- 
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.3a6b472ec20f8e7525a8481f9e2378a3%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

[Django] #28490: Descriptors on Models are reported as nonexistent by System Check Framework for ModelAdmin.list_display if they return None

[Django]

#28490: Descriptors on Models are reported as nonexistent by System Check 
Framework
for ModelAdmin.list_display if they return None
-+-
   Reporter:  Hunter |  Owner:  nobody
  Richards   |
   Type:  Bug| Status:  new
  Component: |Version:  master
  contrib.admin  |   Keywords:  admin, descriptor,
   Severity:  Normal |  system, checks, framework,
   Triage Stage: |  validation
  Unreviewed |  Has patch:  1
Needs documentation:  0  |Needs tests:  0
Patch needs improvement:  0  |  Easy pickings:  0
  UI/UX:  0  |
-+-
 = Brief Description

 The Django ModelAdmin System Checks will erroneously mark as invalid any
 reference in `list_display` to a Descriptor on that ModelAdmin's model
 that returns `None` when accessed on that model's class, even if the
 Descriptor would return non-null values in the resulting ListView.

 This error is due to code that subtly conflates a reference to a field
 descriptor with the return value of that descriptor.  In this ticket, I
 give an in-depth explanation of the bug and a short fix, presented inline,
 for demonstration purposes, and a larger fix containing an exciting
 refactor and a justification for it.


 = Example

 Given a model with a field that is a Descriptor that returns `None`:

 {{{#!python
 class DescriptorThatReturnsNone(object):
 def __get__(self, obj, objtype):
 return None

 def __set__(self, obj, value):
 pass


 class ValidationTestModel(models.Model):
 some_other_field = models.CharField(max_length=100)
 none_descriptor = DescriptorThatReturnsNone()
 }}}

 and a ModelAdmin that includes that Descriptor field in `list_display`:

 {{{#!python
 class TestModelAdmin(ModelAdmin):
 list_display = ('some_other_field', 'none_descriptor')
 }}}

 then the system checks that run at project start will fail to find that
 Descriptor attribute with the following error message:

 {{{
 (admin.E108) The value of 'list_display[4]' refers to 'none_descriptor',
 which is not a callable, an attribute of 'TestModelAdmin', or an attribute
 or method on 'modeladmin.ValidationTestModel'.
 }}}


 = Location of Error

 The location of the error is in the following code:

 
https://github.com/django/django/blob/335aa088245a4cc6f1b04ba098458845344290bd/django/contrib/admin/checks.py#L586-L607

 {{{#!python
 elif hasattr(model, item):
 # getattr(model, item) could be an X_RelatedObjectsDescriptor
 try:
 field = model._meta.get_field(item)
 except FieldDoesNotExist:
 try:
 field = getattr(model, item)
 except AttributeError:
 field = None

 if field is None:
 return [
 checks.Error([...]
 }}}

 We follow the branch for `has_attr(model, item)`, and `field =
 model._meta.get_field(item)` throws an error, so we call `field =
 getattr(model, item)` for the purpose of either getting a reference to
 this non-field attribute or its value itself, depending on the type of
 `item`.  Supposedly, if `getattr` were to throw an error, we'd set `field`
 to none and return E108 (more on this below), but in the Descriptor case
 above, we don't.  But `field` is still `None`, i.e. the return value of
 the descriptor, so E108 is triggered anyway, even though the Descriptor
 field is a perfectly valid value to display in a ListView.

 The cause of the error comes from confusing the "type" of `field`: when
 using Descriptors, it is not always the case that `getattr(SomeClass,
 some_attr_name)` will return a reference to the field in question and
 `getattr(some_class_instance, some_attr_name)` will return the actual
 value of the attribute, which is the unstated assumption of the quoted
 code.  Therefore, `field` sometimes references a field itself, and
 sometimes a field's value.


 = A Workaround and a Quick Fix

 I triggered this error with a slightly more complicated Descriptor that
 returned the value of a related field on its declared model, and `None` if
 that were not possible.  Realizing the special value `None` was at the
 heart of the error, as a workaround I rewrote the Descriptor to return the
 empty string as an "empty" value instead of `None` which is
 indistinguishable from `None` in a ListView.

 As an example of how I intend to fix the error, here's The Simplest Thing
 that Could Possibly Work:

 {{{#!diff
 diff --git a/django/contrib/admin/checks.py
 b/django/contrib/admin/checks.py
 index 830a190..b6b49a5 100644
 --- a/django/contrib/admin/checks.py
 +++ b/django/contrib/admin/checks.py