Opened 3 years ago

Closed 2 years ago

#17271 closed New feature (fixed)

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

Reported by: andreypopp Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: 8mayday@…, fizista@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (1)

none_custom_qs.patch (3.5 KB) - added by andreypopp 3 years ago.
Patch for ticket #17271

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by andreypopp

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Bug to New feature

comment:2 Changed 3 years ago by andreypopp

  • Cc 8mayday@… added

Changed 3 years ago by andreypopp

Patch for ticket #17271

comment:3 follow-up: Changed 3 years ago by Wojciech Banaś <fizista@…>

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by Wojciech Banaś <fizista@…>

  • Cc fizista@… added

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by 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...

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by 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

comment:7 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by andreypopp

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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by 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?

comment:9 follow-up: Changed 3 years ago by lukeplant

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

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.

comment:10 in reply to: ↑ 8 Changed 3 years ago by andreypopp

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.

comment:11 in reply to: ↑ 9 Changed 3 years ago by carljm

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:12 Changed 3 years ago by lukeplant

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

comment:13 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

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

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

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