Fix bug where descriptor class missed instance=None case #822
Conversation
…ut in metaprogramming in internals of djangorestframework.
hwalinga
force-pushed
the
account-for-get-historical-object-on-class
branch
from
7144152 to
a8b7cb1
Compare
hwalinga
changed the title
|
@barm Would you be able to take a look at this to merge it? It is just a very minor two lines fix in a wrongly implemented descriptor class. |
|
@rossmechanic Would you be able to see if this fix can be merged? It are just two lines solving an annoying issue with a wrongly implemented descriptor class. I saw you had previous commits affecting the surrounding lines. |
| @@ -579,6 +579,8 @@ def __init__(self, model, fields_included): | |||
| self.fields_included = fields_included | |||
|
|
|||
| def __get__(self, instance, owner): | |||
| if instance is None: | |||
There was a problem hiding this comment.
@hwalinga Why would you want to return the descriptor?
There was a problem hiding this comment.
Oh I see your stackoverflow post. Got it.
There was a problem hiding this comment.
Yes, it is so that any metaprogramming done on the class knows what kind of attribute this is. (ie a descriptor class) You can also return None, but this way you allow anyone inspecting the class (programmatically) to know what is in here.
That is also the bug I got in DRF. There was some metaprogramming that failed to continue when it got to this attribute.
Codecov Report
@@ Coverage Diff @@
## master #822 +/- ##
==========================================
- Coverage 97.87% 97.67% -0.20%
==========================================
Files 18 18
Lines 988 990 +2
Branches 150 151 +1
==========================================
Hits 967 967
- Misses 9 10 +1
- Partials 12 13 +1
Continue to review full report at Codecov.
|
Description
Return self if called on the class instead of instance to not error when used with djangorestframework
Some metaprogramming magic in djangorestframework performs a check on getting all the
propertyobject attributes on a django model. As this loops over all attributes it will also arrive at thehistory_objectwhich is implemented with a descriptor. However, this implementation does not account for being accessed on the class instead of the model. (Soinstanceis None.)This PR just returns self when that happens. This does not interfere with this metaprogramming done, and makes django-simple-history usable again in combination with djangorestframework.
Returning self in
__get__when instance is None is the canonical way of doing this, as explained here:https://stackoverflow.com/questions/29436716/why-do-you-need-if-instance-is-none-in-get-of-a-descriptor-class
Related Issue
Fixes #821
Motivation and Context
Fixed #821
How Has This Been Tested?
I have created the fix and installed it in a project that suffered from this bug and it worked. I don't think we should be pulling in djangorestframework for this?
Types of changes
Descriptor now accounts for being accessed on the class not the instance.
Checklist:
make formatcommand to format my codeAUTHORS.rstCHANGES.rst