Opened 9 years ago

Closed 5 years ago

#25361 closed Bug (fixed)

Unpickling of QuerySet fails in presence of abstract intermediate model

Reported by: Torsten Bronger Owned by: nobody
Component: Database layer (models, ORM) Version: 1.8
Severity: Normal Keywords:
Cc: bronger@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider the following model hierarchy:

from django.db import models

class Pizza(models.Model):
    timestamp = models.DateTimeField("last modified", auto_now=True)

class SpecialPizza(Pizza):
    class Meta:
        abstract = True
        ordering = ["timestamp"]

class MyPizza(SpecialPizza):
    pass

class Topping(models.Model):
    pizza = models.ForeignKey(MyPizza, related_name="toppings")

    class Meta:
        ordering = ["pizza"]

QuerySet objects containing MyPizza objects with Toppings cannot be pickled and unpickled:

my_pizza = models.MyPizza.objects.create()
my_pizza.toppings.add(models.Topping())
pickled = pickle.dumps(my_pizza.toppings.all())
response = pickle.loads(pickled)

The traceback is:

Traceback:
File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in get_response
  132.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in inner
  145.                     return func(*args, **kwargs)
File "/tmp/myproject/myproject/myapp/views.py" in test
  9.     response = pickle.loads(pickled)
File "/usr/lib/python2.7/pickle.py" in loads
  1383.     return Unpickler(file).load()
File "/usr/lib/python2.7/pickle.py" in load
  858.                 dispatch[key](self)
File "/usr/lib/python2.7/pickle.py" in load_reduce
  1134.         value = func(*args)
File "/usr/local/lib/python2.7/dist-packages/django/db/models/fields/__init__.py" in _load_field
  66.     return apps.get_model(app_label, model_name)._meta.get_field(field_name)
File "/usr/local/lib/python2.7/dist-packages/django/apps/registry.py" in get_model
  202.         return self.get_app_config(app_label).get_model(model_name.lower())
File "/usr/local/lib/python2.7/dist-packages/django/apps/config.py" in get_model
  162.                 "App '%s' doesn't have a '%s' model." % (self.label, model_name))

Exception Type: LookupError at /
Exception Value: App 'myapp' doesn't have a 'specialpizza' model.

If one removes one of the "ordering" fields, the unpickler uses "MyPizza" instead of "SpecialPizza", which is correct and should always be the case.

Change History (6)

comment:1 by Simon Charette, 9 years ago

Triage Stage: UnreviewedAccepted

I guess we should special case fields of abstract models in Field.__reduce__.

The code added in #19635 could be either adjusted to raise a RuntimeError or return another unpickling function that relies on the __path__ of the abstract model to retrieve the associated class and call _meta.get_field() on it.

comment:2 by Samuel Spencer, 8 years ago

I just got bit by this and I can't say I've come across the best solution, but I was able to resolve it with this code in /django/db/models/fields/__init__.py:

--- __init__.py	2016-09-22 06:07:36.167725186 +0000
+++ __init__.py.old	2016-09-22 06:08:19.175742789 +0000
@@ -66,11 +66,6 @@
     return apps.get_model(app_label, model_name)._meta.get_field(field_name)
 
 
-def _load_field_for_abstract(app_label, model_name, field_name):
-    from django.utils.module_loading import import_string
-    return import_string('%s.models.%s'%(app_label, model_name))._meta.get_field(field_name)
-
-
 # A guide to Field parameters:
 #
 #   * name:      The name of the field specified in the model.
@@ -508,11 +503,7 @@
             # Deferred model will not be found from the app registry. This
             # could be fixed by reconstructing the deferred model on unpickle.
             raise RuntimeError("Fields of deferred models can't be reduced")
-        if self.model._meta.abstract:
-            func = _load_field_for_abstract
-        else:
-            func = _load_field
-        return func, (self.model._meta.app_label, self.model._meta.object_name,
+        return _load_field, (self.model._meta.app_label, self.model._meta.object_name,
                              self.name)
 
     def get_pk_value_on_save(self, instance):

__init__.py.old is the original from Django 1.8.14. I'm not sure where to begin with submitting and testing this, but I'd like to help if someone could give me some pointers.

Version 0, edited 8 years ago by Samuel Spencer (next)

comment:3 by Baptiste Mispelon, 5 years ago

I believe this got fixed somewhat accidentally.

Using the same model definitions as in the report, I wrote the following testcase:

class TicketTestCase(TestCase):
    def test_ticket(self):
        my_pizza = MyPizza.objects.create()
        my_pizza.toppings.create()

        pickled = pickle.dumps(my_pizza.toppings.all())

        with self.assertRaises(LookupError):
            response = pickle.loads(pickled)

This allowed me to bisect and find the commit that apparently fixed the issue: 67cf5efa31acb2916034afb15610b700695dfcb0.

I'm not very familiar with pickling in Python and that commit doesn't seem to have anything to do with pickling (though it does relate to abstract models) so I'm a bit hesitant to mark this ticket as fixed.

comment:4 by Simon Charette, 5 years ago

Thanks for the sleuthing Baptiste!

I think 67cf5efa31acb2916034afb15610b700695dfcb0 happened to address the crash because it replaced parent links previously bound to abstract models to the actual concrete model subclass. Since only concrete models are registered the apps.get_model(app_label, model_name) now correctly works.

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 200cd880:

Refs #25361 -- Added test for pickling queryset of abstract-inherited models with Meta.ordering.

Fixed in 67cf5efa31acb2916034afb15610b700695dfcb0.

comment:6 by Mariusz Felisiak, 5 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top