Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#15363 closed Cleanup/optimization (fixed)

get_queryset & get_query_set

Reported by: sorl Owned by: loic84
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: loic84, Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (5)

get_query_set.patch (4.4 KB ) - added by sorl 13 years ago.
0001-Fixed-15363-Renamed-get_query_set-methods-by-get_que.patch (55.0 KB ) - added by Simon Charette 11 years ago.
ticket-15363-deprecated-metaclass.diff (59.3 KB ) - added by Simon Charette 11 years ago.
ticket-15363-renamed_method_factory (59.0 KB ) - added by Simon Charette 11 years ago.
ticket-15363-normalize-get_queryset.patch​ (92.0 KB ) - added by loic84 11 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 by anonymous, 13 years ago

Version: 1.21.3-beta

comment:2 by Russell Keith-Magee, 13 years ago

Component: UncategorizedCore framework
milestone: 2.0
Triage Stage: UnreviewedAccepted

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.

comment:3 by sorl, 13 years ago

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?

comment:4 by sorl, 13 years ago

Has patch: set

Added patch for class based generic views to use get_query_set

by sorl, 13 years ago

Attachment: get_query_set.patch added

comment:5 by Russell Keith-Magee, 13 years ago

Has patch: unset

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.

comment:6 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:7 by Jacob, 12 years ago

Milestone 2.0 deleted

comment:3 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:4 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:5 by Aymeric Augustin, 11 years ago

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.

comment:6 by loic84, 11 years ago

Owner: changed from nobody to loic84
Status: newassigned

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?

comment:7 by loic84, 11 years ago

Triage Stage: AcceptedDesign decision needed
Version: 1.3-betamaster

comment:8 by Simon Charette, 11 years ago

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


comment:9 by Simon Charette, 11 years ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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.

comment:10 by loic84, 11 years ago

Cc: loic84 added

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.

comment:11 by Simon Charette, 11 years ago

Cc: Simon Charette added

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.

by Simon Charette, 11 years ago

comment:12 by Simon Charette, 11 years ago

Needs tests: unset
Patch needs improvement: unset

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.

Last edited 11 years ago by Simon Charette (previous) (diff)

comment:13 by loic84, 11 years ago

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.

comment:14 by sorl, 11 years ago

Cc: mikko@… removed

comment:15 by Anssi Kääriäinen, 11 years ago

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.

by Simon Charette, 11 years ago

comment:16 by Simon Charette, 11 years ago

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 :)

comment:17 by loic84, 11 years ago

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

Last edited 11 years ago by loic84 (previous) (diff)

comment:18 by loic84, 11 years ago

Needs documentation: unset

Added documentation on github.

comment:19 by Simon Charette, 11 years ago

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

comment:21 by Simon Charette <charette.s@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 6983a1a540a6e6c3bd941fa15ddd8cb49f9ec74e:

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

comment:22 by anonymous, 11 years ago

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.

comment:23 by loic84, 11 years ago

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.

comment:24 by anonymous, 11 years ago

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.

comment:25 by Anders Steinlein, 11 years ago

Resolution: fixed
Status: closednew

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?

comment:26 by Simon Charette, 11 years ago

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?

comment:27 by loic84, 11 years ago

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

comment:28 by Anders Steinlein, 11 years ago

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

comment:29 by Simon Charette, 11 years ago

Resolution: fixed
Status: newclosed

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

comment:30 by anonymous, 11 years ago

@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!

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