get_queryset & get_query_set

[sorl]

There are references to methods/functions of both names get_query_set and get_queryset which I think means all the same in all cases, should this not have one and the same name? I would suggest conforming to get_query_set.

$ grep -lr "get_query_set" *
contrib/contenttypes/generic.py
contrib/admin/views/main.py
contrib/admin/options.py
contrib/gis/db/models/manager.py
contrib/sites/managers.py
contrib/databrowse/datastructures.py
contrib/databrowse/plugins/calendars.py
contrib/comments/templatetags/comments.py
contrib/comments/managers.py
db/models/manager.py
db/models/fields/related.py
forms/models.py

$ grep -lr "get_queryset" *
contrib/admin/helpers.py
forms/models.py
shortcuts/__init__.py
views/generic/edit.py
views/generic/detail.py
views/generic/dates.py
views/generic/list.py

[Russell Keith-Magee]

Accepted that we should be consistent. However, it's going to be extremely difficult to make the change in a backwards compatible way. That makes it an activity for the 2.0 timeframe.

[sorl]

if its too late i totally understand, but perhaps changing get_queryset for get_query_set in generic views is possible before 1.3 final?

[sorl]

Added patch for class based generic views to use get_query_set

[Russell Keith-Magee]

I've taken a closer look at this, and .

Firstly, get_queryset() is consistent with PEP8, so that *should* be the preferred name.

Secondly, if we changed to using get_query_set() in Class-based views, we would have a situation where we have self.queryset, but self.get_query_set(). Inconsistency is a bad thing, but internal inconsistency is worse.

So, it's an annoying inconsistency, but it's one we'll live with until 2.0.

[Jacob]

Milestone 2.0 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Aymeric Augustin]

By now, as far as I can tell, the consensus has shifted towards not making a break-everything 2.0. I think we can proceed with a regular deprecation path.

We should normalize towards the non-underscored version: queryset and get_queryset because that's the most common way of writing it.

[loic84]

I've been working on a patch that wraps the various queryset getter (ie. queryset(), get_query_set(), etc.) with the following decorator:

def rename_queryset_getter(original_func):
    """
    Deprecate older method names for get_queryset(). See ticket #15363.
    """
    def deprecated_func(*args, **kwargs):
        warnings.warn(
            ('Please use %s instead.' % original_func.__name__),
            PendingDeprecationWarning, 2)
        return original_func(*args, **kwargs)
    return deprecated_func

All tests pass, albeit with a lot of expected deprecation warnings. The issue is this doesn't provide an upgrade path for people who override these methods in class inheritance, their methods will be silently ignored.

I'm not sure how to proceed at this point, should we close this ticket or go forward and document the change as backward incompatible?

[Simon Charette]

The following class decorator should nail it.

def deprecated_get_query_set(cls):
    def get_query_set(self, *args, **kwargs):
        warnings.warn(
            ('`%s.get_query_set` is deprecated, use `get_queryset` instead.' % cls.__name__),
            PendingDeprecationWarning, 2)
        return cls.get_queryset(self, *args, **kwargs)
    cls.get_query_set = get_query_set
    return cls

[Simon Charette]

Added a preliminary test which pass on Python 2.7.3/3.2.3 SQLite without raising a warning.

It requires testing that the classes decorated with @deprecated_get_query_set correctly raises a PendingDeprecatingWarning, a release note and documentation fixes.

[loic84]

Unless I'm missing the point, I think this decorator suffers from the same issue as mine: since all calls to get_query_set in Django core are changed to get_queryset, the user defined get_query_set will never be called.

Also I went the method decorator route because there are a few more queryset getter inconsistencies such as BaseModelAdmin.queryset, get_prefetch_query_set, etc.

[Simon Charette]

Indeed, I overlooked this issue. If we want to maintain backward compatibility, especially in inheritance scenarios this is going to be quite hard.

Here's ​an attempt at this by overriding __getattribute__.

We could also achieve this by __metaclass__ but I'm afraid this might break something.

[Simon Charette]

I gave a try at the metaclass approach and it works great.

Introduced a DeprecatedGetQuerySet type which does the following:

• Issue a warning on class creation if get_query_set is defined.
• Raise an ImproperlyConfigured error if both get_queryset and get_query_set are defined.
• Define a get_queryset method if not present.
• Define or wrap the existing get_query_set to complain when called.

The added tests pass on Python 2.7.3/3.2.3 SQLite without raising a warning.

[loic84]

This approach is pretty clever and seems to work well.

Should we aim at removing the last few occurrences of query_set for the sake of consistency? There are only a few variables and methods left after this patch, mostly private APIs.

Also BaseModelAdmin.queryset is what brought me to this ticket: I wasted a bunch of time and resorted to some crazy hacks to access the admin queryset because I failed to notice this method since I was looking for a get_queryset. All similar methods in ModelAdmin have a get_ prefix.

For completeness I'll also mention ListFilter.queryset but I believe this one is acceptable since it is consistent with the surrounding methods.

I think this patch is a good opportunity to solve the rest of the queryset accessor inconsistencies.

[Anssi Kääriäinen]

It is still possible to break this by mixins or plain method assignment. Of these mixins might be actually used in the wild.

The mixins example is:

class ManagerMixin(object):
    def get_query_set(self):
        ...

class MyManager(ManagerMixin, models.Manager):
    pass

according to very quick testing this doesn't work similarly in master and after patch.

It is likely possible to make mixins work with some mro introspection. I don't think the direct assignment case needs to be supported, it is likely extremely rare.

Apart of the above cases I can't think of any case that will not work - as long as each class in the inheritance chain is subclass of Manager they will get both the get_queryset and get_query_set methods, and things will work. But, Python allows for some crazy constructs so I am not surprised if other problematic constructs are found.

I am not sure if this kind of renaming is the best idea in the world. But, I don't care enough to actually put a -0 here (let alone -1). I guess we could do this renaming, and if we want to do other similar ones, then lets first wait for 1.6 and see how this one actually worked out. Of course, if something similar has been done already and it worked well, then put me into +0 camp.

[Simon Charette]

Added a new patch that doesn't solve the mixin issue described by @akaariai but provide a generic of dealing with method rename deprecations.

It should be easier now to use it for the get_prefetch_query_set -> get_prefetch_queryset and queryset -> get_queryset migration if it should ever happen.

Will wait for another core developer to chime in about whether or not it's worth the trouble to go forward with this.

Personally I think the change is worth it for the sake on consistency. Pushing the first rename (get_query_set -> get_queryset) early into the 1.6 life cycle should give us a clear view over the viability of this kind of refactoring.

@loic84, I won't have time to work on this in next few weeks. Feel free to tackle the __mro__ issues :)

[loic84]

I had a go at the mro introspection.

@charettes, I somehow failed to notice your patch with the factory and I came up with something slightly different but in the same spirit. It shouldn't be too hard to replace it with the factory. If you like it better, just let me know.

The metaclass works a bit differently though; no exception is raised when both the old and the new methods are present, since it actually rely on the existence of both.

The logic goes as follows:

1. Define the new method if missing and complain about it.
2. Define the old method if missing to assure backwards compatibility.
3. Complain whenever an old method is called.

The full test suite runs without warning (with -Wall). I also updated every reference to the deprecated methods in the docs except in the releases subfolder.

I pushed my branch on github to make it easier to review.

​https://github.com/loic/django/compare/ticket15363

[loic84]

Added documentation on github.

[Simon Charette]

Could you open a PR of this branch against django:master, would be easier to review.

[loic84]

PR ​https://github.com/django/django/pull/867

[Simon Charette <charette.s@…>]

In 6983a1a540a6e6c3bd941fa15ddd8cb49f9ec74e:

Fixed #15363 -- Renamed and normalized to get_queryset the methods that return a QuerySet.

[anonymous]

Loïc, I have just had an issue with the patch you submitted.
I don't understand well metaclasses, but the following now breaks with metaclass conflict :
​https://github.com/dominno/django-moderation/blob/master/src/moderation/managers.py

Maybe one could tell what is wrong and how to fix this.

[loic84]

I'm not sure why you would use this temporary metaclass, could you try the following:

    def __call__(self, base_manager, *args, **kwargs):
        return type(
            self.__class__.__name__,
            (self.__class__, base_manager),
            {'use_for_related_fields': True}
        )

Note that I didn't manage to run your test suite, even with the old TEST_RUNNER so it's just a guess.

[anonymous]

Loïc, you were right, and I understand a little better the conflict issue. Just using type worked like a charm. Thank you so much.

[Anders Steinlein]

I'm testing one of our apps with 1.6 beta 1, and went ahead and changed queryset to get_queryset in our model admins and custom list filters. The former works fine, however, it turns out that list filters is still using the old queryset instead of get_queryset as I expected. Shouldn't this cleanup patch change that as well?

[Simon Charette]

Can you point us to a place where queryset is called from contrib.admin? Are you sure it's not a third-party application that is using the deprecated API?

[loic84]

@asteinlein, is it about ListFilter.queryset?

We decided against renaming that one because it would be inconsistent with the rest of the methods of that class which don't use the get_ prefix.

[Anders Steinlein]

@loic84, @charettes: Yes, I'm referring to ListFilter.queryset. I now see that you discussed that method and kept it unchanged for internal consistency. I am however in disagreement with that choice, as people will be thinking "how do I get the queryset? I'm guessing get_queryset as that's whats used everywhere else", not "I'm guessing queryset, as that matches the other methods on list filters"... IMHO, of course, but I was bitten by this exact thing implementing my own custom list filter.

[Simon Charette]

@asteinlein I disagree with your people will be thinking assertion. I think most of Django users will be referring to ​the documentation when subclassing SimpleListFilter since it's a newly introduced feature.

However I agree that the whole ListFilter method names (missing get_) might be confusing for people used to override BaseModelAdmin methods thus I suggest you open new ticket suggesting to prefix ListFilter methods. It should be easy to write a patch using the RenameMethodsBase and we should be more confident shipping another method rename once people got their hands on Django 1.6 and played with the get_query_set one.

[anonymous]

@charettes Thanks for your response and suggestion. I've added this to my todo, so I'll open a separate ticket and provide a patch for this after 1.6 has shipped. Thanks again!