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

Django 1.10 default manager not MultilingualManager for model inheriting abstract class with a manager defined on objects (leading to AttributeError: 'QuerySet' object has no attribute 'rewrite') #389

Closed
nealtodd opened this issue Oct 19, 2016 · 24 comments

Comments

@nealtodd
Copy link

Whilst upgrading my application to Django 1.10 I came across the following issue where the default manager for a Model Translation registered model didn't get dynamically set to modeltranslation.manager.MultilingualManager.

This resulted in an exception being thrown in update_translation_fields.py L32

AttributeError: 'QuerySet' object has no attribute 'rewrite'

A similar issue was resolved in #123 but it seems something in the internal changes in Django 1.10 has resurfaced it in the particular scenario I have (not one covered by #381).

I haven't yet identified what the changes are that cause it but I've narrowed the scenario down to one in which a model inherits from an abstract model that explicitly sets a manager via the objects attribute. In this case the resulting _default_manager is that set on objects rather than modeltranslation.manager.MultilingualManager.

The simplest example I can boil it down to is this:

# models.py

class FooAbstract(models.Model):
    class Meta:
        abstract = True

class Foo(FooAbstract):
    name = models.CharField(blank=True, max_length=1)

class Bar(models.Model):
    objects = models.Manager()
    name = models.CharField(blank=True, max_length=1)

class BazAbstract(models.Model):
    objects = models.Manager()
    class Meta:
        abstract = True

class Baz(BazAbstract):
    name = models.CharField(blank=True, max_length=1)


# translations.py

@register(Foo)
class FooTranslationOptions(TranslationOptions):
    fields = ('name',)

@register(Bar)
class BarTranslationOptions(TranslationOptions):
    fields = ('name',)

@register(Baz)
class BazTranslationOptions(TranslationOptions):
    fields = ('name',)


# shell

from modeltranslation.translator import translator
models = translator.get_registered_models(abstract=False)
for model in models:
    print(model.__name__, type(model._default_manager))

# Django 1.9 output:

Foo <class 'modeltranslation.manager.MultilingualManager'>
Bar <class 'modeltranslation.manager.MultilingualManager'>
Baz <class 'modeltranslation.manager.MultilingualManager'>

# Django 1.10 output:

Foo <class 'modeltranslation.manager.MultilingualManager'>
Bar <class 'modeltranslation.manager.MultilingualManager'>
Baz <class 'django.db.models.manager.Manager'>

I don't have a solution yet but thought I'd submit it as an issue now in case anyone has any ideas?

@zlorf
Copy link
Collaborator

zlorf commented Oct 19, 2016

Which version of MT are you using?

@nealtodd
Copy link
Author

Sorry, I forgot to add that info! It's the latest 0.12 from PyPi.

@Ge0
Copy link

Ge0 commented Oct 19, 2016

Python 3.5.2
Django 1.10.2
Django-Modeltranslation 0.12
sqlite3

We are told here django/django@ed0ff91 that:

Old API:

class CustomManager(models.Model):
    use_for_related_fields = True

class Model(models.Model):
    custom_manager = CustomManager()

New API:

class Model(models.Model):
    custom_manager = CustomManager()

    class Meta:
        base_manager_name = 'custom_manager'

On the following file - https://github.com/zlorf/django-modeltranslation/blob/master/modeltranslation/manager.py#L141 - we can see this:

class MultilingualManager(models.Manager):
    use_for_related_fields = True

    def get_query_set(self):
return MultilingualQuerySet(self.model)

This snippet seems to abide by the "Old API" instead of the new one. Might be related to this problem?

@nealtodd
Copy link
Author

@Ge0 I did wonder whether that and inheritance changes documented in the 1.10 release notes were the source of the change, but they are deprecations in 1.10 so the pre-1.10 behaviour should still remain:

During the deprecation period, use_for_related_fields will be honored and raise a warning, even if a base_manager_name is set. This allows third-party code to preserve legacy behavior while transitioning to the new API.

@nealtodd
Copy link
Author

The 1.10 release notes don't mention it, but manager inheritance rules have changed between 1.9 and 1.10 which is likely to affect the chosen default manager:

Compare Django 1.10 Custom managers and model inheritance with Django 1.9 Custom managers and model inheritance

@Ge0
Copy link

Ge0 commented Oct 20, 2016

Maybe we have to take a look at the register decorator whose purpose is overriding the decorated class with the custom MultilingualManager manager, then?

https://github.com/deschler/django-modeltranslation/blob/master/modeltranslation/translator.py#L423

I am sorry if this comment is useless. I try to find leads to fix this issue and my poor python knowledge helps me from getting things done quickly at the moment.

@Ge0
Copy link

Ge0 commented Oct 24, 2016

I might have found another lead here:

https://docs.djangoproject.com/en/1.10/ref/models/options/#django.db.models.Options.default_manager_name

Since the version 1.10, the _default_manager attribute of a model is an instance of the class defined by _meta.default_manager.

Therefore I think of adding a meta.default_manager to our model, set to either MultilingualManager or the NewMultilingualManager created in case when we have a custom manager in our own models.

What do you guys think?

@zlorf
Copy link
Collaborator

zlorf commented Oct 24, 2016

I need a moment of free time to take a deep look into the issue; probably will take care of it at the incoming weekend.

@logileifs
Copy link

Is there any way around this while this gets fixed ?

@nealtodd
Copy link
Author

nealtodd commented Dec 8, 2016

A workaround that seems to work for me (without detailed testing) is to redeclare objects on the subclass. This then results in the subclass's default manager being MultilingualManager.

E.g., using my examples from the first comment of this issue:

class BazAbstract(models.Model):
    objects = models.Manager()
    class Meta:
        abstract = True

class Baz(BazAbstract):
    objects = models.Manager()  # <- This has been added
    name = models.CharField(blank=True, max_length=1)

# Django 1.10 output:
Baz <class 'django.db.models.manager.MultilingualManager'>

(I'm redeclaring objects on Baz rather than just moving it off BazAbstract to Baz because in my real models the abstract class is being used by other non-modeltranslation enabled classes and I don't want to touch them (and, in general, one might not be able to change the abstract class anyway).)

Fingers crossed for a fix, Model Translation is so useful!

@nealtodd
Copy link
Author

A caveat to my previous comment about a workaround. This only applies to the default manager on the subclass. Other managers inherited from the abstract base class won't be patched to become subclasses of MultilingualManager, so take care if using them with translation-registered fields because they won't work as expected (they'll just use the original non-language-suffixed database field).

@logileifs
Copy link

is there any update on this? @zlorf have you started working on this issue yet?

@melvyn-sopacua
Copy link

Looking at how the new default_manager() method in django.db.models.options.Options works, it should be enough to specify default_manager_name for any concrete object. patch_metaclass seems the appropriate place to do this, but not familiar enough with the inner workings of modeltranslation to assess all consequences.

@melvyn-sopacua
Copy link

Workaround: I've monkey-patched Mezzanine like below and two models started working. Of course got stuck on another that didn't use Displayable, but fix should be identical.

--- /usr/local/lib/python3.5/site-packages/mezzanine/core/models.py.orig       2016-09-26 01:49:47.000000000 +0200
+++ /usr/local/lib/python3.5/site-packages/mezzanine/core/models.py     2017-01-26 23:08:47.244624046 +0100
@@ -216,6 +216,15 @@
 SHORT_URL_UNSET = "unset"


+def wrapped_manager(klass):
+    if settings.USE_MODELTRANSLATION:
+        from modeltranslation.manager import MultilingualManager
+        class Mgr(MultilingualManager, klass):
+            pass
+        return Mgr()
+    else:
+        return klass()
+
 class Displayable(Slugged, MetaData, TimeStamped):
     """
     Abstract model that provides features of a visible page on the
@@ -236,7 +245,7 @@
     short_url = models.URLField(blank=True, null=True)
     in_sitemap = models.BooleanField(_("Show in sitemap"), default=True)

-    objects = DisplayableManager()
+    objects = wrapped_manager(DisplayableManager)
     search_fields = {"keywords": 10, "title": 5}

     class Meta:

@melvyn-sopacua
Copy link

Full fix for Mezzanine 4.2.2 for reference.

@FilippoIOVINE
Copy link

I am using Zinnia and fix above didn't work for me so I figured out another workaround:
I added at the end of my local translator.py file the following code (just after the translator.register(...) line)

from modeltranslation.manager import MultilingualManager, MultilingualQuerysetManager
from modeltranslation.translator import has_custom_queryset

def patch_manager_class(manager):
    if isinstance(manager, MultilingualManager):
        return
    if manager.__class__ is Manager:
        manager.__class__ = MultilingualManager
    else:
        class NewMultilingualManager(MultilingualManager, manager.__class__,
                                     MultilingualQuerysetManager):
            use_for_related_fields = getattr(
                manager.__class__, "use_for_related_fields", not has_custom_queryset(manager))
            _old_module = manager.__module__
            _old_class = manager.__class__.__name__

            def deconstruct(self):
                return (
                    False,  # as_manager
                    '%s.%s' % (self._old_module, self._old_class),  # manager_class
                    None,  # qs_class
                    self._constructor_args[0],  # args
                    self._constructor_args[1],  # kwargs
                )

        manager.__class__ = NewMultilingualManager

patch_manager_class(Entry.objects)
patch_manager_class(Entry.published)

I extracted code above directly from modeltranslation/translator.py and used it to reapply the modeltranslation manager patch to original managers from the abstract class.

Anyway debugging modeltranslation/translator.py code

    managers = (model._meta.local_managers if NEW_MANAGER_API else
                (getattr(model, x[1]) for x in
                 model._meta.concrete_managers + model._meta.abstract_managers))
    for current_manager in managers:
        prev_class = current_manager.__class__
        patch_manager_class(current_manager)
        if model._default_manager.__class__ is prev_class:
            # Normally model._default_manager is a reference to one of model's managers
            # (and would be patched by the way).
            # However, in some rare situations (mostly proxy models)
            # model._default_manager is not the same instance as one of managers, but it
            # share the same class.
            model._default_manager.__class__ = current_manager.__class__
    patch_manager_class(model._base_manager)
    if hasattr(model._meta, "_expire_cache"):
        model._meta._expire_cache()

I noticed:

  1. model._meta.local_managers is always empty
  2. that changing it to model._meta.managers (it's holding right managers list instead) it worked
  3. finally model._meta._expire_cache() destroy patch just installed by 2).

Moving away code related to point 3) (before managers = (model._meta.local_managers if NEW_MANAGER_API... for example) apparently fixed the issue in the original code but I am not sure if this would work for all cases.

@SalahAdDin
Copy link

Now we have django 1.11 and this bug isn't solved yet.

@madEng84
Copy link

I think that documenting the add of the manager used in the subclass could be a good workaround

@merwok
Copy link

merwok commented Jul 3, 2017

I am having this issue in models that combine django-polymorphic with modeltranslation. The objects redefinition workaround fixed update_translation_fields; I am checking if the admin needs more work.

Schnouki added a commit to Schnouki/django-modeltranslation that referenced this issue Sep 26, 2017
With several level of inheritance, custom managers with custom
querysets, and a ManyToMany field with an explicit intermediary table.

This reproduces bugs deschler#389 and deschler#413.
@dmarcelino
Copy link

dmarcelino commented Nov 23, 2017

@Schnouki, @zlorf, @deschler, PR #431 fixed the issue for me.

While using v0.12.1 with Django v1.10 I was getting the error:

./manage.py update_translation_fields

Updating data of model '<class 'myproject.products.models.Product'>'
Traceback (most recent call last):
  File "./manage.py", line 23, in <module>
    execute_from_command_line(sys.argv)
  File "/home/dario/development/myproject/.venv/lib/python3.5/site-packages/django/core/management/__init__.py", line 367, in execute_from_command_line
    utility.execute()
  File "/home/dario/development/myproject/.venv/lib/python3.5/site-packages/django/core/management/__init__.py", line 359, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/dario/development/myproject/.venv/lib/python3.5/site-packages/django/core/management/base.py", line 294, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/dario/development/myproject/.venv/lib/python3.5/site-packages/django/core/management/base.py", line 345, in execute
    output = self.handle(*args, **options)
  File "/home/dario/development/myproject/.venv/lib/python3.5/site-packages/modeltranslation/management/commands/update_translation_fields.py", line 32, in handle
    model._default_manager.filter(q).rewrite(False).update(
AttributeError: 'SafeDeleteQueryset' object has no attribute 'rewrite'

I believe this is due to my Product model inheriting from SafeDeleteMixin (django-safedelete).

After installing modeltranslation from @Schnouki's branch the command worked flawlessly.

@nealtodd
Copy link
Author

@Schnouki @deschler Brilliant, thank you!

@merwok
Copy link

merwok commented Dec 6, 2017

Please tell us when this is released! :)

@wonderbeyond
Copy link

Does Django 2.x have with this bug?

@last-partizan
Copy link
Collaborator

That was probably not related to django, and by now this bug must be fixed.

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

No branches or pull requests