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

[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

[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! :)

--
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+unsubscribe@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

[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

> <https://github.com/django/django/commit/d2a26c1a90e837777dabdf3d67ceec4d2a70fb86#diff-507b415116b409afa4f723e41a759a9eL550>

> 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 <za...@meat.io <mailto:za...@meat.io>>

> 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

> <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
> <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

> <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-develop...@googlegroups.com
> <mailto:django-develop...@googlegroups.com>.

> To post to this group, send email to

> django-d...@googlegroups.com
> <mailto:django-d...@googlegroups.com>.

> Visit this group at
> https://groups.google.com/group/django-developers

> <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

> <https://groups.google.com/d/msgid/django-developers/ec0e2506-4c4b-441f-b005-5623d6521ae3%40googlegroups.com?utm_medium=email&utm_source=footer>.

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

> <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-develop...@googlegroups.com
> <mailto:django-develop...@googlegroups.com>.
> To post to this group, send email to django-d...@googlegroups.com
> <mailto:django-d...@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
> <https://groups.google.com/d/msgid/django-developers/CAMyDDM3ZqsL5JStFfuGG%3DM%2BAmOcmkOLGAkmTRDkVfUYXnMqcYA%40mail.gmail.com?utm_medium=email&utm_source=footer>.

[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

--

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+unsubscribe@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.

[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

[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

--
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+unsubscribe@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