[Django] #28381: New _or_create() field validation prevents certain kinds of descriptor use
Django
-------------------------------------+-------------------------------------
Reporter: Kevin | Owner: nobody
Christopher Henry |
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
My code stopped working after upgrading to Django 1.11.2. The cause lies
in
[https://github.com/django/django/commit/b9abdd92ab21a796943e8047e778eebc671c8c00
b9abdd92], the fix for #28222.
The root of the issue is that the validation code checks the model to see
which attributes are properties. To do that it loops over all the
attribute names and calls `getattr()` on them, which triggers any
descriptors you've defined on the model. So if you have a descriptor that
is calling a Manager method that does this validation, you will end up in
an infinite loop (`maximum recursion depth exceeded`). I personally use
such descriptors to create lazy model attributes that access the database,
and find them quite useful.
I'm not sure offhand of a simple way to maintain the validation behavior
without triggering all the descriptors. You could check the Model's
`__dict__` rather than calling `getattr()` but of course that won't work
with inheritance. This might just be a `wontfix` but I wanted to at least
document the change in behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Could you possibly make your descriptors return `self` when `__get__` is
called with `instance=None` instead of performing a query? The other
solution would be to lookup `model.__dict__` but that would require
`__mro__` handling.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:1>
Django
-------------------------------------+-------------------------------------
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Christopher Henry):
In my case the purpose of the descriptors is to do the lookup on the
class, when the instance is `None`. For example, I want to say
`Thing.SPECIAL_THING` to access a specific instance. Touching the database
at import time is forbidden, so instead I make this a descriptor on
`Thing` that does the lookup on `__get__()`.
It would be nice to preserve the ability to run such queries from a
descriptor. But even putting aside database lookups and the recursion
error I think it's reasonably idiomatic to put expensive operations behind
a descriptor, and it will definitely be surprising to find those
descriptors accessed every time you do an unrelated `get_or_create()`.
That said, I haven't thought of a good solution, and writing a `getattr()`
alternative (as you say, going through the `mro` looking at `dicts`)
sounds a bit hairy.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:2>
Django
-------------------------------------+-------------------------------------
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Out of curiosity, is there a reason the class specific lookup is
implemented as a class-descriptor instead of a
`classmethod(get_special_thing)`?
I agree that Django should try to play it safer in regards to these
objects but I'm asking because we've made our descriptors return `self` to
make life easier for introspection tools (e.g. `help()`) in the past.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:3>
Django
-------------------------------------+-------------------------------------
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Christopher Henry):
I did it that way simply because it makes for a nicer API for users of the
class (a typical descriptor use case), hiding implementation details (like
how Django is initialized) and presenting a simple interface for accessing
these constant instances.
Certainly it's not essential, and there are other ways of doing the same
sort of thing. I think it would be reasonable policy to say that Django's
introspection requirements are such that you should expect that your model
attributes can be accessed at any time. That would preclude many
interesting uses of descriptors accessed on the model class, but if
documented would avoid the kind of surprise I received.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:4>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: nobody
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:5>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: nobody
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
marfire, can you confirm that replacing the `getattr` call with
`inspect.getattr_static` solves your issue? I initially skimmed the
`inspect` documentation with a vague feeing it was providing such feature
but couldn't find the actual function until
[https://github.com/django/django/pull/6721#issuecomment-316924192 Carl's
mention a few hours ago].
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:6>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: nobody
Henry |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Christopher Henry):
Ah, good find, that's just what we need. I can confirm that
`getattr_static()` doesn't trigger the descriptor, so using it instead of
`getattr()` for validation should solve this problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:7>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: Simon
Henry | Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:8>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: Simon
Henry | Charette
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => duplicate
Comment:
Well this seem to have already been fixed in 1.11.3 by
ed244199c72f5bbf33ab4547e06e69873d7271d0 (#28269).
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:9>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: Simon
Henry | Charette
Type: Bug | Status: closed
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Christopher Henry):
Not
[https://github.com/django/django/blob/1.11.3/django/db/models/options.py#L888
1.11.3] (where I originally saw this, before tracing it back to 1.11.2),
but in master. Good news, though.
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:10>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: Simon
Henry | Charette
Type: Bug | Status: closed
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam (Chainz) Johnson (added)
Comment:
Unfortunately `getattr_static` is only available in Python 3.2+ and 1.11
still supported Python 2.7. I guess the patch could be adjusted to try to
use `getattr_static` on Python 3.2 but I'm not sure it's worth it given
the rare condition under which this occurs. Any thoughts on that Adam?
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:11>
Django
descriptor use
-------------------------------------+-------------------------------------
Reporter: Kevin Christopher | Owner: Simon
Henry | Charette
Type: Bug | Status: closed
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
@charettes the backport from master didn't use getattr_static:
https://github.com/django/django/commit/b7d6077517c6cb2daa5e5faf2ae9f94698c06ca9
--
Ticket URL: <https://code.djangoproject.com/ticket/28381#comment:12>