Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#28381 closed Bug (duplicate)

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

Reported by: Kevin Christopher Henry Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.11
Severity: Normal Keywords:
Cc: Simon Charette, Adam (Chainz) Johnson Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (12)

comment:1 Changed 16 months ago 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.

comment:2 Changed 16 months ago 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.

comment:3 Changed 16 months ago 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.

comment:4 Changed 16 months ago 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.

comment:5 Changed 16 months ago by Tim Graham

Summary: New _or_create() field validation prevents certain kinds of descriptor useQuerySet.get/update_or_create() field validation prevents certain kinds of descriptor use
Triage Stage: UnreviewedAccepted

comment:6 Changed 16 months ago by Simon Charette

Cc: Simon Charette added

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.

comment:7 Changed 16 months ago 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.

comment:8 Changed 16 months ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:9 Changed 16 months ago by Simon Charette

Resolution: duplicate
Status: assignedclosed

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

comment:10 Changed 16 months ago by 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.

comment:11 Changed 16 months ago by Simon Charette

Cc: Adam (Chainz) Johnson added

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?

comment:12 Changed 16 months ago by Adam (Chainz) Johnson

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

Note: See TracTickets for help on using tickets.
Back to Top