Ticket #15363 -- Normalization of the queryset methods name

[charettes]

Since the consensus as shifted toward not making a break-everything 2.0 I'm planning to normalize the queryset methods name early in the 1.6 development cycle to spot any breakages.

The affected classes are the following:

• django.contrib.admin.options.BaseModelAdmin (queryset -­> get_queryset).
• django.contrib.admin.views.main.ChangeList (get_query_set -> get_queryset).
• django.contrib.comments.templatetags.comments.BaseCommentNode (get_query_set -> get_queryset).
• django.contrib.contenttypes.generic.GenericForeignKey (get_prefetch_query_set -> get_prefetch_queryset).
• django.db.models.fields.related.SingleRelatedObjectDescriptor (get_query_set -­­> get_queryset, get_prefetch_query_set -> get_prefetch_queryset).
• django.db.models.fields.related.ReverseSingleRelatedObjectDescriptor (get_query_set -­­> get_queryset, get_prefetch_query_set -> get_prefetch_queryset).
• django.db.models.manager.Manager (get_query_set -­­> get_queryset, get_prefetch_query_set -> get_prefetch_queryset).

And subclasses (ModelAdmin, ...) where bold items are part of the documented API and "->" stands for rename.

The patch does the following:

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.

As discussed in the ticket the whole MRO is handled by the patch so mixins should also work correctly. The only issue you might encounter is if you defined a custom metaclass for one of the classed affected by the rename. The simple fix is to make sure your metaclass subclasses the class of the class you're planning to attach it to:

class MyManagerBase(Manager.__class__):
    pass

class MyManager(Manager):
    __metaclass__ = MyManagerBase
 

If your code base or library must support both Django > 1.6 and <= 1.6 you should define both methods until you drop support for < 1.6:

class MyManager(Manager):
    def get_queryset(self, *args, **kwargs):
        # Do something funky
        # ...

    def get_query_set(self, *args, **kwargs):
        # TODO: Remove when support for Django < 1.6 is dropped.
        return MyManager.get_queryset(self, *args, **kwargs)
 

The pull request can be reviewed here .

I'm planning to commit the proposed fix this week if no major issues are raised here or on the ticket.

[Jacob Kaplan-Moss]

Hm.

I'm +1 on cleaning up the names.... but do we *really* have to use a
metaclass for this? Seems to me this is "gratuitous use of a
metaclass" territory, especially given the shenanigans you have to go
through with subclassing the metaclass.

I'd be a lot happier with this patch if it just did this in a simple
manner: have the old methods raise the warnings and call the new ones.
Unless there's a actual need for the metaclassing beyond saving a few
lines of code can you drop it, please?

Jacob

> --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Aymeric Augustin]

There's a good, although non-obvious, reason.

Without the metaclass, a method overridden in the subclass with the old name would be silently skipped.

--
Aymeric.

[charettes]

Unfortunately this would break in subtle in confusing ways in inheritance scenarios.

Say you've defined the following Manager subclass prior to Django 1.6 and we're only raising a warning on Manager.get_query_set calls.:

class ActiveManager(models.Manager):
    def get_query_set(self, *args, **kwargs):
        return super(ActiveManager, self).get_query_set(*args, **kwargs).filter(is_active=True)

You would get a Manager.get_query_set() is deprecated, use `get_queryset()` instead. warning upon calling ActiveManager.get_query_set, so far so good.

From here you would go ahead and rename your super call and/or rename ActiveManager.get_query_set to get_queryset:

class ActiveManager(models.Manager):
    def get_query_set(self, *args, **kwargs):
        return super(ActiveManager, self).get_queryset(*args, **kwargs).filter(is_active=True)

Would be broken since internal code calls get_queryset. And renaming both:

class ActiveManager(models.Manager):
    def get_queryset(self, *args, **kwargs):
        return super(ActiveManager, self).get_queryset(*args, **kwargs).filter(is_active=True)

Would not be backward compatible with code not yet migrated (third-party apps for example) that still calls get_query_set. The correct way of handling this by hand would be to define both methods when you override a renamed one:

class ActiveManager(models.Manager):
    def get_queryset(self, *args, **kwargs):
        return super(ActiveManager, self).get_queryset(*args, **kwargs).filter(is_active=True)

    def get_query_set(self, *args, **kwargs):
        return ActiveManager.get_queryset(self, *args, **kwargs)

Again we could raise a more verbose warning telling the user to do exactly that but that would break if you're subclassing a Manager subclass from a third party app that didn't migrate yet:

class ThirdPartyManager(models.Manager):
    """You cannot do the rename here since it's from a third party (site-packages,...) module"""
    def get_query_set(self, *args, **kwargs):
        qs = super(ThirdPartyManager, self).get_query_set(*args, **kwargs)
        # To something with qs...
        return qs

And you would be left to yourself figuring out you have to do the following:

class MyManager(ThirdPartyManager):
    def get_queryset(self, *args, **kwargs):
        # Call ThirdPartyManager.get_query_set because it didn't migrate yet.
        qs = super(MyManager, self).get_query_set(*args, **kwargs)
        # To something with qs...
        return qs

    def get_query_set(self, *args, **kwargs):
        # Compatiblity shim
        return MyManager.get_queryset(self, *args, **kwargs)

Maintaining backward compatiblity with third party apps is the main reaosn a class decorator wouldn't work here. @python_2_unicode_compatible works well because __str__ and __unicode__ are not often involved in super trickery while you almost always override get_queryset when you subclass Manager.

The use of the metaclass make sure you're not shooting yourself in the foot all and all the metaclass subclassing trouble should only concern a minority of users who most probably know what they're doing.

Simon

[Jacob Kaplan-Moss]

On Wed, Mar 6, 2013 at 12:21 PM, charettes <chare...@gmail.com> wrote:
> The use of the metaclass make sure you're not shooting yourself in the foot
> all and all the metaclass subclassing trouble should only concern a minority
> of users who most probably know what they're doing.

Ah, OK, that's the missing part I was, um, missing.

Thanks for schooling me, and I withdraw my objection :)

Jacob