QueryManager constraints don't apply to RelatedManagers #31

Open
jonashaag opened this Issue Mar 15, 2013 · 6 comments

3 participants

@jonashaag

What?

Let's say you have a model like this:

class Article(models.Model):
    author = models.ForeignKey('Author')
    published = models.BooleanField()

    published_only = QueryManager(published=True)

Note that published_only is the default manager here.

Now if you do

an_author.article_set...

the published=True constraint is NOT added to the query.

Why?

When you access a related model that way, Django internally subclasses the model's default manager (db/models/fields/related.py, ForeignRelatedObjectsDescriptor:related_manager_cls) and uses that manager for the query. The issue here is that by the current implementation, django-model-utils' QueryManager keeps the constraints as instance state and not as class state. Which obviously means that the constraints are lost in the Django-generated related manager.

Solution 1

Fix this in Django: The Django-generated manager class should not only extend the default manager's class but the concrete instance. By not using inheritance for example.

Solution 2

Make QueryManager(...) call actually generate a subclass that incorporates the constraints (arguments). Probably easier but doesn't feel "right".

@carljm
Owner

Yes, this was also a problem for PassThroughManager and we fixed it with Solution 2, which as you say is ugly but works (requires some care to avoid breaking pickling). That's certainly possible here as well and I'd accept a pull request for it.

Solution 1 is an interesting proposal for Django; it would certainly make it easier to implement managers that are customized at the instance level and allow them to work for related managers as well. Speaking as a Django core dev, I'd welcome it if you filed a ticket for that at code.djangoproject.com.

@jonashaag

https://code.djangoproject.com/ticket/20057

I won't code up any of this though.

@rouge8

This is also still a problem for PassThroughManager unless you use it with the PassThroughManager.for_queryset_class()() syntax. However, if you inherit from PassThroughManager and override get_query_set somewhere in your manager's inheritance tree, you can't use the for_queryset_class style.

I have the general structure of my models in a gist. MyModel.objects returns a queryset with only the latest versions and with the passthrough methods, as it should. However, RelatedManagers don't keep the passthrough methods, as expected. If I use the for_queryset_class style, the get_query_set method of my LatestVersionManager is never called.

Is there a reasonable way to approach this problem?

@carljm
Owner

It might be possible to fix for_queryset_class() so it doesn't ignore the superclass version of get_query_set. Specifically I'm thinking that Queryset._clone() accepts a klass argument to supply a new class for the cloned queryset. So instead of just instantiating and returning a new queryset of class queryset_cls, The get_query_set() method of the dynamically created manager class could instead call super, get its queryset, and then clone it; something like::

def get_query_set(self):
    qs = super(_PassThroughManager, self).get_query_set()
    return qs._clone(klass=queryset_cls)

Not entirely positive this would work, but it might be worth a try.

Really this is all quite ugly and hacky, and the best solution would be for someone to take a serious run at https://code.djangoproject.com/ticket/20057 and fix the problem at the source. That'll be more work.

@rouge8

Yeah, that works and doesn't break any of the existing tests. Should I make a pull request with that change and a test case?

@rouge8 rouge8 added a commit to rouge8/django-model-utils that referenced this issue May 24, 2013
@rouge8 rouge8 PassThroughManager calls superclass `get_query_set`.
As discussed in #31, PassThroughManager and _PassThroughManager would
ignore the superclass version of `get_query_set`.

Test soon.
97c09a3
@carljm
Owner

Looks good to me! A pull request would be great; just add a test, add a note in CHANGES.rst, and add yourself to AUTHORS. I don't think this requires any updates to the documentation in README.rst; it already recommends for_queryset_class().

@rouge8 rouge8 added a commit to rouge8/django-model-utils that referenced this issue May 26, 2013
@rouge8 rouge8 PassThroughManager calls superclass `get_query_set`.
As discussed in #31, PassThroughManager and _PassThroughManager would
ignore the superclass version of `get_query_set`.
b94b5e8
@rouge8 rouge8 added a commit to rouge8/django-model-utils that referenced this issue May 26, 2013
@rouge8 rouge8 PassThroughManager calls superclass `get_query_set`.
As discussed in #31, PassThroughManager and
PassThroughManager.for_queryset_class() would ignore the superclass
version of `get_query_set`.
7e38117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment