Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using cls.__dict__ sometimes returns <class 'django.db.models.manager.ManagerDescriptor'> #369

Closed
benjaoming opened this issue Apr 22, 2015 · 10 comments · Fixed by #372
Closed

Comments

@benjaoming
Copy link
Contributor

I'm having problems that I didn't get to the bottom of, except that reverting back to getattr instead of accessing via __dict__ is the remedy.

Problem was that a custom manager was not set, and we were defaulting back to TreeManager.

The culprit was this:

                        # HACK: avoid using getattr(cls, attr)
                        # because it calls __get__ on descriptors, which can cause nasty side effects
                        # with more inconsiderate apps.
                        # (e.g. django-tagging's TagField is a descriptor which can do db queries on getattr)
                        # ( ref: http://stackoverflow.com/questions/27790344 )
                        obj = cls.__dict__[attr]

It was causing django-wiki/django-wiki#404

PR coming in......

@benjaoming
Copy link
Contributor Author

Here's the exact cause: a01b157

But I realize it fixes another important issue.

@craigds
Copy link
Member

craigds commented Apr 22, 2015

Hi Ben

The last comment on the django-wiki ticket implies this is something to do with .none() querysets, but I'm not quite sure how that relates to ManagerDescriptor.

Perhaps you could you clarify a bit?

@benjaoming
Copy link
Contributor Author

@craigds thanks for getting back

I had to halt the investigation, but I'm trying to figure it out. I was building a test case for django-mptt but it somehow succeeds (oh no!!! type(Model.objects.all()[0].get_children()) is CustomQuerySet) with the current codebase, but the exact same thing in django-wiki is failing with the same structure (type(Model.objects.all()[0].get_children()) is TreeQuerySet instead of CustomQuerySet).

Human rant: So the problem is caused by django-wiki's custom manager... it something isn't playing the right way, since __dict__[attr] is returning a ManagerDescriptor (that's a class that will just raise an exception about not using manager functions directly on an object), and then django-mptt defaults to TreeManager because it doesn't find the custom manager inheriting from TreeManager.

@benjaoming
Copy link
Contributor Author

Btw thanks for a great project again!!!

@benjaoming
Copy link
Contributor Author

Okay I've found the problem, it's essentially this:

class MyModel(MPTTModel):
    objects = MyCustomManager()
    _default_manager = objects  # <- THAT IS THE PROBLEM

@benjaoming
Copy link
Contributor Author

...now I just need to figure out why that line was introduced

@benjaoming
Copy link
Contributor Author

Confirming that django-wiki is working after commenting out the _default_manager = objects line.

@benjaoming
Copy link
Contributor Author

Also confirming that I can make the django-mptt tests break by introducing this line... wondering if it's relevant since googling stuff about this issue reveals issues about older django versions.

@benjaoming
Copy link
Contributor Author

Okay, it's all in #370

I would add that it seems removing _default_manager in the model in django-wiki isn't harmful, I'll try it out and report back.

@benjaoming
Copy link
Contributor Author

Argh, dammit, I wanted Travis to be showing off how the tests were failing in 2b47ffd and then how they are now working in 92737e9

LazarBekcic added a commit to LazarBekcic/django-mptt that referenced this issue Dec 16, 2019
…ts for custom TreeManagers.

Squashed commit of the following:

commit d55f90c8f0b0b6c90a91ba04b1db1e34af099593
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 02:32:20 2015 +0200

    re-enable _default_manager and remove test that fails for some other reason on django 1.4 django-mptt/django-mptt#371

commit 3939f182564c2d1b32a70efb7a5c910234da3887
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 02:20:58 2015 +0200

    Oh hi Travis, can you check this again to see how Django 1.4 likes _default_manager ?

commit 17957e03d2b6f9abbf7db890914194489af2929e
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 02:09:20 2015 +0200

    also assert that the classic version of .none() is compatible with the custom manager's queryset

commit 1cf707d0dae042387f3fae1e34739860f1dbdf2d
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:59:22 2015 +0200

    fix for django-mptt/django-mptt#371 by customizing test to behave differently if it's pre django 1.6

commit fcf90deeaf1f4e4d4a3bb9e8be265cd4e475d576
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:47:20 2015 +0200

    Re-add _default_manager but uncomment test that's generally failing on django 1.4

commit 073698bf2f13b5ab3b77916ce434a46a183c83f6
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:26:59 2015 +0200

    Hi Travis, please test if this works with Django 1.4 because it didnt B4

commit 04308a02f3a0fb2b3e2e9a8a55593de330c3aaaa
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:13:41 2015 +0200

    Renaming custom manager in doctests

commit 92737e9890babe7f310ad0bd4b0594d49c000eb8
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:04:19 2015 +0200

    Add optimization: If conventional "objects" attr exists, just use that and don't iterate everything else, also fixes django-mptt/django-mptt#369

commit 2b47ffd611158fa91c44398e584b2c5f1f1e96bf
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:03:42 2015 +0200

    Add test cases that fail when using _default_manager and objects django-mptt/django-mptt#369

commit 1ba49283070e2fda7d00bdd2680a0f9a039c890f
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:02:25 2015 +0200

    Add a custom method and a _default_manager that points to "objects" to reflect a practice that may or may not exist out there in the wild django-mptt/django-mptt#369

commit 00933d01a3974c088330fb77bfeb7e65dbecf185
Author: Benjamin Bach <benjaoming@gmail.com>
Date:   Thu Apr 23 01:01:56 2015 +0200

    add a single row on the Person test model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants