Opened 2 years ago

Closed 23 months ago

Last modified 22 months ago

#20289 closed Bug (fixed)

Unpickling of dynamic model classes broken in 1.5

Reported by: akaariai Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
Severity: Release blocker Keywords:
Cc: charettes Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The commit 146aff3bac974e56ec8cb597c2720d1cd4f77b26 broke unpickling of dynamically created models. This was reported in django-users thread http://groups.google.com/group/django-users/browse_thread/thread/1eee4a4d5098bfc5.

The problem is that deferred classes aren't only possible source of dynamically created class in Django. ManyToMany through class are dynamically created and in addition dynamic classes are possible in user code. My understanding is that user code dynamic classes should be supported, too.

I am not sure how to trigger this bug using standard Django models and fields. The django-users thread mentions this:

So for example, I have an instance of a Student, which has a
ForeignKey to Book, which in turn has a ManyToMany to Author.
If I try and cache my Student instance, I get that pickling
error.

but I haven't tested this.

I think resurrecting the simple_class_factory path of code instead of using super.__reduce__ for the non deferred case is the right fix. Of course, the code comments need fixing, too, so just reverting the commit isn't a good idea.

Note that I haven't done any actual testing (no Django available on this machine...), so I will leave this unreviewed until I (or somebody else) will reproduce this using Django 1.5.

Change History (9)

comment:1 Changed 2 years ago by charettes

  • Cc charettes added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 2 years ago by MeirKriheli

Any news regarding that ? It breaks caching, had to be disabled for now, which takes it's toll from sites.

Thanks

comment:3 Changed 2 years ago by akaariai

I think I have something that fixes this, see: https://github.com/akaariai/django/commit/6730c97f96fe1377fe81a8e4d089257bd36e3df6

However those test cases do not pass on 1.4 (see https://github.com/akaariai/django/tree/ticket_20289_14). So, I would like to see a reproducible regression between 1.4 and 1.5 so that it is sure we got the regression fixed.

comment:4 Changed 2 years ago by andrewgodwin

Yes, I can't get anything to pass on 1.4 either, either using a container or direct type() calls with models and various combinations of app_label and __module__.

Provided the tests do now pass on trunk, though, I'm reasonably happy to call this ticket fixed, though perhaps we should add something like this to the test too (as it's what the mailing list says was happening):

meta = type("Meta", (object, ), {"app_label": "queryset_pickle"})
original = type("MyTestModel", (models.Model, ), {"Meta": meta, "__module__": Group.__module__, ...fields...})
dumped = pickle.dumps(original)
reloaded = pickle.loads(dumped)
self.assertEqual(original, reloaded)

comment:5 Changed 2 years ago by akaariai

  • Triage Stage changed from Accepted to Ready for checkin

I guess we can just commit this one with the added test? I'll mark this RFC, the test of comment:4 should be likely added, but that is easy enough to do when committing...

comment:6 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5459795ef224c5c81461c06a95d38390ee91f014:

Fixed #20289 -- pickling of dynamic models

comment:7 Changed 2 years ago by akaariai

  • Resolution fixed deleted
  • Status changed from closed to new
  • Triage Stage changed from Ready for checkin to Accepted

I don't feel comfortable backpatching this unless absolutely needed. The report was that things break when upgrading from 1.4 to 1.5, but I haven't been able to found any case where we have a model pickling regression.

Could it be that the breakage isn't actually due to changes in model pickling, but trying to load 1.4 pickled data with 1.5 unpickle. We don't guarantee pickling compatibility between versions, so if this is the case then we shouldn't backpatch (not that backpatching even helps here).

I will reopen this as 1.5 release blocker, lets see what we need to do to this before 1.5.2.

comment:8 Changed 23 months ago by akaariai

  • Resolution set to fixed
  • Status changed from new to closed

I think it is time to close this one. This ticket is still missing an example of the claimed regression. I believe the problem is actually in trying to use model pickling across versions. In my understanding that isn't officially supported.

comment:9 Changed 22 months ago by almostabc

I know this bug has been closed, but I'm running into the same thing and am 99% sure it isn't a cross-version pickling issue. I'm working on a simple test case that demonstrates this problem and will update this bug when it is ready.

Note: See TracTickets for help on using tickets.
Back to Top