Code

Opened 3 years ago

Closed 8 months ago

Last modified 8 months ago

#15363 closed Cleanup/optimization (fixed)

get_queryset & get_query_set

Reported by: sorl Owned by: loic84
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: loic84, charettes 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 3 years ago.
0001-Fixed-15363-Renamed-get_query_set-methods-by-get_que.patch (55.0 KB) - added by charettes 15 months ago.
ticket-15363-deprecated-metaclass.diff (59.3 KB) - added by charettes 15 months ago.
ticket-15363-renamed_method_factory (59.0 KB) - added by charettes 14 months ago.
ticket-15363-normalize-get_queryset.patch​ (92.0 KB) - added by loic84 14 months ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 3 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.2 to 1.3-beta

comment:2 Changed 3 years ago by russellm

  • Component changed from Uncategorized to Core framework
  • milestone set to 2.0
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by 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?

comment:4 Changed 3 years ago by sorl

  • Has patch set

Added patch for class based generic views to use get_query_set

Changed 3 years ago by sorl

comment:5 Changed 3 years ago by russellm

  • 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 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 3 years ago by jacob

Milestone 2.0 deleted

comment:3 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 15 months ago by aaugustin

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 Changed 15 months ago by loic84

  • Owner changed from nobody to loic84
  • Status changed from new to assigned

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 Changed 15 months ago by loic84

  • Triage Stage changed from Accepted to Design decision needed
  • Version changed from 1.3-beta to master

comment:8 Changed 15 months ago by charettes

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 Changed 15 months ago by charettes

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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 Changed 15 months ago by loic84

  • 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 Changed 15 months ago by charettes

  • Cc charettes 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.

Changed 15 months ago by charettes

comment:12 Changed 15 months ago by charettes

  • 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 15 months ago by charettes (previous) (diff)

comment:13 Changed 14 months ago by 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.

comment:14 Changed 14 months ago by sorl

  • Cc mikko@… removed

comment:15 Changed 14 months ago by akaariai

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.

Changed 14 months ago by charettes

comment:16 Changed 14 months ago by charettes

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 Changed 14 months ago by 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

Version 0, edited 14 months ago by loic84 (next)

comment:18 Changed 14 months ago by loic84

  • Needs documentation unset

Added documentation on github.

comment:19 Changed 14 months ago by charettes

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

comment:21 Changed 13 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 6983a1a540a6e6c3bd941fa15ddd8cb49f9ec74e:

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

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

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

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

comment:25 Changed 9 months ago by asteinlein

  • Resolution fixed deleted
  • Status changed from closed to new

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 Changed 9 months ago by charettes

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 Changed 9 months ago by 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.

comment:28 Changed 9 months ago by asteinlein

@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 Changed 8 months ago by charettes

  • Resolution set to fixed
  • Status changed from new to closed

@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 Changed 8 months ago by 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!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.