Skip to content
This repository has been archived by the owner on Nov 28, 2024. It is now read-only.
This repository has been archived by the owner on Nov 28, 2024. It is now read-only.

get_query_set patching is very broken #29

Closed
@spookylukey

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

spookylukey commented on Jun 13, 2015

@spookylukey
Author

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

NotSqrt commented on Jun 26, 2015

@NotSqrt

I am using django 1.7 with perfectly valid managers, and the monkey patch adds the warning "Manager.get_query_set method should be renamed get_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> and Manager.__dict__.get("get_queryset") is None.

Disabling the monkey patch removes the warning, but doing the following also does !! :

try:
    Manager.get_query_set = Manager.get_queryset
    Manager.get_queryset = Manager.get_queryset  # yep !
except AttributeError:
    Manager.get_queryset = Manager.get_query_set
spookylukey

spookylukey commented on Jul 8, 2015

@spookylukey
Author

Can I point out that this issue is a major data loss bug?

If you get get_queryset wrong, then Django's dynamic RelatedManager 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 returning Room.objects.downstairs() instead of just the rooms that are related to the house. So if you do house.rooms.downstairs().delete(), you just deleted far more things than you intended to (Similarly an update() 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 the RelatedManager classes, which are dynamically created.

philippeowagner

philippeowagner commented on Jul 8, 2015

@philippeowagner
Member

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.

spookylukey

spookylukey commented on Jul 8, 2015

@spookylukey
Author

Great, thanks! I just didn't want this to get lost, since it is a potential data loss situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      get_query_set patching is very broken · Issue #29 · arteria/django-compat