get_query_set patching is very broken #29
Description
The method included to patch get_query_set is very broken, and leads to some very bug code in circumstances where managers get subclassed - the behaviour of the subclass can be skipped. This is a very serious error for the case of Related managers, because it means that the limiting of the queryset that is added by the subclass get_query_set method gets skipped, so the related manager queries apply to all objects.
I've created a branch that demonstrates the problem, adding 4 tests, 2 of which fails with Django 1.8, one fails with Django 1.5:
https://github.com/spookylukey/django-compat/tree/get_query_set_patch_bug
I've created a PR so that you can see the tests failing.
The full details are explained in this blog post:
http://lukeplant.me.uk/blog/posts/handling-django's-get_query_set-rename-is-hard/
Activity
spookylukey commentedon Jun 13, 2015
By the way, I don't think there is a good way to fix this. But it would be better to remove the support for
get_query_set
/get_queryset
than advertise something that is flawed.NotSqrt commentedon Jun 26, 2015
I am using django 1.7 with perfectly valid managers, and the monkey patch adds the warning "
Manager.get_query_set
method should be renamedget_queryset
".After some inspection, it appears that at some point during the django init, the Manager class gets to a point where
Manager.__dict__.get("get_query_set")
is<unbound method Manager.get_queryset>
andManager.__dict__.get("get_queryset")
isNone
.Disabling the monkey patch removes the warning, but doing the following also does !! :
spookylukey commentedon Jul 8, 2015
Can I point out that this issue is a major data loss bug?
If you get
get_queryset
wrong, then Django's dynamicRelatedManager
stops working.This is shown by the failing tests in my branch: master...spookylukey:get_query_set_patch_bug
In my example,
house.rooms.downstairs()
starts returningRoom.objects.downstairs()
instead of just the rooms that are related to the house. So if you dohouse.rooms.downstairs().delete()
, you just deleted far more things than you intended to (Similarly anupdate()
method affects far more records than intended).Please fix this bug, by removing the
get_query_set
monkey patching. That method can't work, since it can't monkey patch theRelatedManager
classes, which are dynamically created.philippeowagner commentedon Jul 8, 2015
Sure @spookylukey, we will remove the
get_query_set
support. It simply not has been done yet. We will rework the README (#17, #28) at the same time. Thanks again for your contribution.Removed get_queryset <> get_query_set see, #29
spookylukey commentedon Jul 8, 2015
Great, thanks! I just didn't want this to get lost, since it is a potential data loss situation.