Re: Exceptions in model._meta._property_names with some 3rd-pty libraries

[Adam Johnson]

I've made a pair of PR's, one for Django master and one for the 1.11
branch. On master, which is Python 3+, getattr_static is used .

https://github.com/django/django/pull/8599
https://github.com/django/django/pull/8601

On 3 June 2017 at 01:02, Carl Meyer  wrote:

> Hi Zack,
>
> On 06/02/2017 02:02 PM, Zack Voase wrote:
> > Hi all,
> >
> > I'm encountering exceptions in a number of popular third-party Django
> > libraries when upgrading from 1.8 to 1.11. The code path is typically
> > `MyModel.objects.get_or_create(...)`, which causes
> > `model._meta._property_names` to be checked (to make sure we're not
> > querying/setting a property). The problem is, `_property_names` is
> > implemented as follows:
> >
> > |
> > # in django/db/models/options.py:
> > def_property_names(self):
> > returnfrozenset({
> >attr forattr in
> >dir(self.model)ifisinstance(getattr(self.model,attr),property)
> > })
> > |
> >
> > The problem is when a custom field installs a field descriptor on the
> > model class (during `contribute_to_class()`), with a `__get__()` method
> > like this:
> >
> > |
> > classSomeFieldDescriptor(object):
> > # ...
> > def__get__(self,instance,type=None):
> > ifinstance isNone:
> > raiseAttributeError("Can only be accessed via an instance.")
> > # ...
> > |
>
> I can see two things here that could be done better, one on Django side
> and one on third-party app side.
>
> 1) I've never seen a good reason for a descriptor to raise an
> AttributeError like that. Typically `return self` is a much more useful
> option for the "access on the class" scenario, if the descriptor can't
> provide useful behavior in that case, as it allows introspecting the
> class and finding the descriptor object.
>
> 2) On the Django side, I think we should switch to using
> `inspect.getattr_static`, to avoid any possibility of triggering
> side-effecty descriptor code of any kind. AttributeError is only the
> most visible problem here; there could be much subtler problems (e.g.
> performance issues) caused by e.g. accessing a descriptor that does a
> database query or something.
>
> Carl
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers  (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/django-developers/fbd639ad-6a51-7657-f647-753cbc3b6b90%40oddbird.net
> .
> For more options, visit https://groups.google.com/d/optout.
>



-- 
Adam

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM1Y2a3N0yz4fBNpG9qCKxAcci5QN2Muf2B2eKyN6ebxjw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Exceptions in model._meta._property_names with some 3rd-pty libraries

[Carl Meyer]

Hi Zack,

On 06/02/2017 02:02 PM, Zack Voase wrote:
> Hi all,
> 
> I'm encountering exceptions in a number of popular third-party Django
> libraries when upgrading from 1.8 to 1.11. The code path is typically
> `MyModel.objects.get_or_create(...)`, which causes
> `model._meta._property_names` to be checked (to make sure we're not
> querying/setting a property). The problem is, `_property_names` is
> implemented as follows:
> 
> |
> # in django/db/models/options.py:
> def_property_names(self):
> returnfrozenset({
>attr forattr in
>dir(self.model)ifisinstance(getattr(self.model,attr),property)
> })
> |
> 
> The problem is when a custom field installs a field descriptor on the
> model class (during `contribute_to_class()`), with a `__get__()` method
> like this:
> 
> |
> classSomeFieldDescriptor(object):
> # ...
> def__get__(self,instance,type=None):
> ifinstance isNone:
> raiseAttributeError("Can only be accessed via an instance.")
> # ...
> |

I can see two things here that could be done better, one on Django side
and one on third-party app side.

1) I've never seen a good reason for a descriptor to raise an
AttributeError like that. Typically `return self` is a much more useful
option for the "access on the class" scenario, if the descriptor can't
provide useful behavior in that case, as it allows introspecting the
class and finding the descriptor object.

2) On the Django side, I think we should switch to using
`inspect.getattr_static`, to avoid any possibility of triggering
side-effecty descriptor code of any kind. AttributeError is only the
most visible problem here; there could be much subtler problems (e.g.
performance issues) caused by e.g. accessing a descriptor that does a
database query or something.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/fbd639ad-6a51-7657-f647-753cbc3b6b90%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: Exceptions in model._meta._property_names with some 3rd-pty libraries

[Sergey Fursov]

Hi everyone,

I just want to mention here that in django-jsonfield repo proposed solution
to solve this problem on third party side:
https://github.com/dmkoch/django-jsonfield/issues/189. Maybe this is the
better way.

Thanks,
Sergey

2017-06-03 0:02 GMT+03:00 Zack Voase :

> Hi all,
>
> I'm encountering exceptions in a number of popular third-party Django
> libraries when upgrading from 1.8 to 1.11. The code path is typically
> `MyModel.objects.get_or_create(...)`, which causes
> `model._meta._property_names` to be checked (to make sure we're not
> querying/setting a property). The problem is, `_property_names` is
> implemented as follows:
>
> # in django/db/models/options.py:
> def _property_names(self):
> return frozenset({
>attr for attr in
>dir(self.model) if isinstance(getattr(self.model, attr), property)
> })
>
> The problem is when a custom field installs a field descriptor on the
> model class (during `contribute_to_class()`), with a `__get__()` method
> like this:
>
> class SomeFieldDescriptor(object):
> # ...
> def __get__(self, instance, type=None):
> if instance is None:
> raise AttributeError("Can only be accessed via an instance.")
> # ...
>
> Libraries which install such descriptors include [django-fsm](
> https://github.com/kmmbvnr/django-fsm/blob/2d2eaee/django_fsm/__init__.
> py#L225) and [django-jsonfield](https://github.com/dmkoch/django-
> jsonfield/blob/2.0.1/jsonfield/subclassing.py#L35). I think the problem
> is that we can't assume that all results of `dir(model_class)` can be
> accessed via `getattr(model_class, attr)` without exception; indeed, the
> Python docs state (c.f. https://docs.python.org/2/
> library/functions.html#dir):
>
> > Because dir() is supplied primarily as a convenience for use at an
> interactive prompt, it tries to supply an interesting set of names more
> than it tries to supply a rigorously or consistently defined set of names,
> and its detailed behavior may change across releases.
>
> A potential fix would be to adjust the definition of `_property_names`
> like so:
>
> def _property_names(self):
> attrs = []
> for attr in dir(self.model):
> try:
> if isinstance(getattr(self.model, attr), property):
> attrs.append(attr)
> except AttributeError:
> pass
> return frozenset(attrs)
>
> Does this seem like a good solution, or even a problem worth solving?
>
> Thanks!
> -- Zack
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/django-developers/ec0e2506-4c4b-441f-b005-
> 5623d6521ae3%40googlegroups.com
> 
> .
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAG-zk-BDvJOQfhgNBFBpRCBuPkB7MTtJCYLp6D1jxgbppMdr%2BQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Exceptions in model._meta._property_names with some 3rd-pty libraries

[Curtis Maloney]

What about using inspect.getmembers ?

https://docs.python.org/3/library/inspect.html#inspect.getmembers

In other code I've also used inspect.classify_class_attrs but it seems 
to be undocumented :/


If nothing else, it could be used as a guide on how to do this.

--
Curtis


On 03/06/17 08:52, Adam Johnson wrote:

This is my bad, I did the refactoring :) You're right, the original
version

did in fact use a try..except AttributeError and that should be
preserved for cases like this.

I've made a ticket here https://code.djangoproject.com/ticket/28269 . If
you want to make the PR copying in your fix and adding a test that would
be neat, you've done 90% of the work already! :)


On 2 June 2017 at 22:02, Zack Voase >
wrote:

Hi all,

I'm encountering exceptions in a number of popular third-party
Django libraries when upgrading from 1.8 to 1.11. The code path is
typically `MyModel.objects.get_or_create(...)`, which causes
`model._meta._property_names` to be checked (to make sure we're not
querying/setting a property). The problem is, `_property_names` is
implemented as follows:

|
# in django/db/models/options.py:
def_property_names(self):
returnfrozenset({
   attr forattr in
   dir(self.model)ifisinstance(getattr(self.model,attr),property)
})
|

The problem is when a custom field installs a field descriptor on
the model class (during `contribute_to_class()`), with a `__get__()`
method like this:

|
classSomeFieldDescriptor(object):
# ...
def__get__(self,instance,type=None):
ifinstance isNone:
raiseAttributeError("Can only be accessed via an instance.")
# ...
|

Libraries which install such descriptors include

[django-fsm](https://github.com/kmmbvnr/django-fsm/blob/2d2eaee/django_fsm/__init__.py#L225

)
and

[django-jsonfield](https://github.com/dmkoch/django-jsonfield/blob/2.0.1/jsonfield/subclassing.py#L35

).
I think the problem is that we can't assume that all results of
`dir(model_class)` can be accessed via `getattr(model_class, attr)`
without exception; indeed, the Python docs state (c.f.
https://docs.python.org/2/library/functions.html#dir
):

> Because dir() is supplied primarily as a convenience for use at an
interactive prompt, it tries to supply an interesting set of names
more than it tries to supply a rigorously or consistently defined
set of names, and its detailed behavior may change across releases.

A potential fix would be to adjust the definition of
`_property_names` like so:

|
def_property_names(self):
attrs =[]
forattr indir(self.model):
try:
ifisinstance(getattr(self.model,attr),property):
attrs.append(attr)
exceptAttributeError:
pass
returnfrozenset(attrs)
|

Does this seem like a good solution, or even a problem worth solving?

Thanks!
-- Zack

--
You received this message because you are subscribed to the Google
Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to django-developers+unsubscr...@googlegroups.com
.
To post to this group, send email to
django-developers@googlegroups.com
.
Visit this group at
https://groups.google.com/group/django-developers
.
To view this discussion on the web visit

https://groups.google.com/d/msgid/django-developers/ec0e2506-4c4b-441f-b005-5623d6521ae3%40googlegroups.com

.
For more options, visit https://groups.google.com/d/optout
.




--
Adam

--
You received this message because you are subscribed to the Google
Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to django-developers+unsubscr...@googlegroups.com
.
To post to this group, send email to django-developers@googlegroups.com
.
Visit this group at https://groups.google.com/group/django-developers.
To 

Re: Exceptions in model._meta._property_names with some 3rd-pty libraries

[Adam Johnson]

This is my bad, I did the refactoring :) You're right, the original version

did in fact use a try..except AttributeError and that should be preserved
for cases like this.

I've made a ticket here https://code.djangoproject.com/ticket/28269 . If
you want to make the PR copying in your fix and adding a test that would be
neat, you've done 90% of the work already! :)


On 2 June 2017 at 22:02, Zack Voase  wrote:

> Hi all,
>
> I'm encountering exceptions in a number of popular third-party Django
> libraries when upgrading from 1.8 to 1.11. The code path is typically
> `MyModel.objects.get_or_create(...)`, which causes
> `model._meta._property_names` to be checked (to make sure we're not
> querying/setting a property). The problem is, `_property_names` is
> implemented as follows:
>
> # in django/db/models/options.py:
> def _property_names(self):
> return frozenset({
>attr for attr in
>dir(self.model) if isinstance(getattr(self.model, attr), property)
> })
>
> The problem is when a custom field installs a field descriptor on the
> model class (during `contribute_to_class()`), with a `__get__()` method
> like this:
>
> class SomeFieldDescriptor(object):
> # ...
> def __get__(self, instance, type=None):
> if instance is None:
> raise AttributeError("Can only be accessed via an instance.")
> # ...
>
> Libraries which install such descriptors include [django-fsm](
> https://github.com/kmmbvnr/django-fsm/blob/2d2eaee/django_fsm/__init__.
> py#L225) and [django-jsonfield](https://github.com/dmkoch/django-
> jsonfield/blob/2.0.1/jsonfield/subclassing.py#L35). I think the problem
> is that we can't assume that all results of `dir(model_class)` can be
> accessed via `getattr(model_class, attr)` without exception; indeed, the
> Python docs state (c.f. https://docs.python.org/2/
> library/functions.html#dir):
>
> > Because dir() is supplied primarily as a convenience for use at an
> interactive prompt, it tries to supply an interesting set of names more
> than it tries to supply a rigorously or consistently defined set of names,
> and its detailed behavior may change across releases.
>
> A potential fix would be to adjust the definition of `_property_names`
> like so:
>
> def _property_names(self):
> attrs = []
> for attr in dir(self.model):
> try:
> if isinstance(getattr(self.model, attr), property):
> attrs.append(attr)
> except AttributeError:
> pass
> return frozenset(attrs)
>
> Does this seem like a good solution, or even a problem worth solving?
>
> Thanks!
> -- Zack
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/django-developers/ec0e2506-4c4b-441f-b005-
> 5623d6521ae3%40googlegroups.com
> 
> .
> For more options, visit https://groups.google.com/d/optout.
>



-- 
Adam

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM3ZqsL5JStFfuGG%3DM%2BAmOcmkOLGAkmTRDkVfUYXnMqcYA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.