QuerySet.get/update_or_create() field validation prevents certain kinds of descriptor use

[Kevin Christopher Henry]

My code stopped working after upgrading to Django 1.11.2. The cause lies in ​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.

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

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

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

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

[Simon Charette]

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 ​Carl's mention a few hours ago.

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

[Simon Charette]

Well this seem to have already been fixed in 1.11.3 by ed244199c72f5bbf33ab4547e06e69873d7271d0 (#28269).

[Kevin Christopher Henry]

Not ​1.11.3 (where I originally saw this, before tracing it back to 1.11.2), but in master. Good news, though.

[Simon Charette]

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?

[Adam Johnson]

@charettes the backport from master didn't use getattr_static: ​https://github.com/django/django/commit/b7d6077517c6cb2daa5e5faf2ae9f94698c06ca9