QuerySet.none() doesn't work well with subclasses of QuerySet

[Andrey Popp]

I use QuerySet subclassing to allow building complex composable queries out of smaller ones:

class MyQuerySet(QuerySet):
  def allowed(self, user):
    return self.filter(permissions__in=user.permissions)

  def have_feature(feature_name):
    return self.filter(feature__name=feature_name)
...

objs = Obj.objects.allowed(user).have_feature("somefeature")

Sometimes I want to return EmptyQuerySet by calling none() to provide "fast-path" for some conditions, but unfortunately EmptyQuerySet doesn't have my custom methods, so call chain breaks after returning it:

class MyQuerySet(QuerySet):
  def allowed(self, user):
    if user.is_anonymous:
      return self.none()
    return self.filter(permissions__in=user.permissions)

  def have_feature(feature_name):
    return self.filter(feature__name=feature_name)
...

objs = Obj.objects.allowed(user).have_feature("some feature")
# AttributeError: EmptyQuerySet doesn't have have_feature attribute ...

I've attached a patch which instruments EmptyQuerySet subclass for each particular QuerySet implementation and returns its instance as a result of none() method call. I've also refactored code in Manager.none() to reuse existing code in QuerySet.none() and to parametrize Manager() constructor over QuerySet implementation to use.

[Andrey Popp]

Patch for ticket #17271

[Wojciech Banaś <fizista@…>]

The same does my patch (17270), but the filter is added to the manager and not in queries.

[Wojciech Banaś <fizista@…>]

Replying to Wojciech Banaś <fizista@…>:

The same does my patch (17270), but the filter is added to the manager and not in queries.

I tested also a problem with the method of none () in my solution(17270), and this problem does NOT exist there.

So I think the filters should be located just in manager and not in the QuerySet.

[Andrey Popp]

Replying to Wojciech Banaś <fizista@…>:

I tested also a problem with the method of none () in my solution(17270), and this problem does NOT exist there.

So I think the filters should be located just in manager and not in the QuerySet.

I like to define methods on QuerySet because it allows chain several filters at once:

users = User.objects.have_permission("some permission").is_staff().have_new_messages()

and so on...

[Wojciech Banaś <fizista@…>]

Replying to andreypopp:

Replying to Wojciech Banaś <fizista@…>:

I tested also a problem with the method of none () in my solution(17270), and this problem does NOT exist there.

So I think the filters should be located just in manager and not in the QuerySet.

I like to define methods on QuerySet because it allows chain several filters at once:

users = User.objects.have_permission("some permission").is_staff().have_new_messages()

and so on...

The same you can get by using my patch, and filters are where they should, in the manager.

See my examples in this ticket 17270

[Andrey Popp]

Replying to Wojciech Banaś <fizista@…>:

The same you can get by using my patch, and filters are where they should, in the manager.

See my examples in this ticket 17270

Yep, indeed, I actually wondered if I could provide all methods on Manager and QuerySet simultaneously and haven't found any solutions which look good. Your patch implements this correctly but I think it:

• suffers from performance degradation(very much code are executed during get_query_set() call which isn't rare call, I think)
• doesn't work if Manager implements __getattr__

There's also a strange spot if QuerySet already has some methods — you decided not to override such methods and I think it's good decision while it also makes some implicit assumptions which can confuse users and create new "special case" in documentation.

For me the better and the bigger spot to touch is to completely merge Manager and QuerySet classes into one unit or just expose only on them in API.

[Wojciech Banaś <fizista@…>]

Replying to andreypopp:

Yep, indeed, I actually wondered if I could provide all methods on Manager and QuerySet simultaneously and haven't found any solutions which look good. Your patch implements this correctly but I think it:

• suffers from performance degradation(very much code are executed during get_query_set() call which isn't rare call, I think)
• doesn't work if Manager implements __getattr__

There's also a strange spot if QuerySet already has some methods — you decided not to override such methods and I think it's good decision while it also makes some implicit assumptions which can confuse users and create new "special case" in documentation.

For me the better and the bigger spot to touch is to completely merge Manager and QuerySet classes into one unit or just expose only on them in API.

Your comments were very useful. Have combined your ideas with me, and I received a very short and working code (17270, see attachment manager.3.py).

Do you think that this solution would be good now?

[Luke Plant]

Special casing the none() method doesn't seem very compelling compared to #17270 which addresses the larger problem, therefore closing this bug as a duplicate of #17270.

Thanks.

[Andrey Popp]

Replying to Wojciech Banaś <fizista@…>:

Your comments were very useful. Have combined your ideas with me, and I received a very short and working code (17270, see attachment manager.3.py).

Do you think that this solution would be good now?

It still generates new class at runtime and performs method mangling which can be quite expensive in terms of performance. As I've already said, I suggest to merge Manager and QuerySet functionality into single class or eliminate one of them from public API, probably Manager is the best candidate for this.

[Carl Meyer]

Replying to lukeplant:

Special casing the none() method doesn't seem very compelling compared to #17270 which addresses the larger problem, therefore closing this bug as a duplicate of #17270.

I don't agree with this resolution. I am not at all convinced that the technique in #17270 is a candidate for core (although it can be nice as a third-party module), as it adds a bunch of extra runtime code execution and implicit magical behavior to every single ORM call. And it's not really addressing the same problem at all: it's addressing the difference between a Manager and a QuerySet, not the difference between a QuerySet and an EmptyQuerySet. If you look at the patch there, it only fixes this problem too because it special-cases .none() and EmptyQuerySet every bit as much as a patch for this would. In other words, these are two separate issues, it just happens that #17270 conflates fixes for both of them into a single patch.

In any case, .none() is already special-cased by the existence of EmptyQuerySet - this bug is just looking for a remedy to the consequences of that existing special-case. I think it's a legitimate, and separate, bug.

[Luke Plant]

OK, sorry, I didn't look at the other patch closely.

[Anssi Kääriäinen <akaariai@…>]

In 69a46c5ca7d4e6819096af88cd8d51174efd46df:

Tests for various emptyqs tickets

The tickets are either about different signature between qs.none() and
qs or problems with subclass types (either EmptyQS overrided the custom
qs class, or EmptyQS was overridden by another class - values() did
this).

Fixed #15959, fixed #17271, fixed #17712, fixed #19426