Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25389 closed Bug (fixed)

SimpleLazyObject doesn't pickle correctly when wrapping a Model

Reported by: Ben Kraft Owned by: Ben Kraft
Component: Database layer (models, ORM) Version: 1.8
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given a SimpleLazyObject wrapping an instance of a Django model, if the SimpleLazyObject has not yet been initialized, pickling it will not correctly save the _django_version attribute added by 42736ac8. This happens because SimpleLazyObject's overridden __reduce_ex__ clobbers that of Model, and means that unpickling the pickled object will raise a warning. An example is given here. This is especially relevant given that request.user is a SimpleLazyObject wrapping a model instance, so this will happen when pickling an untouched request.user.

Furthermore, pickling a SimpleLazyObject wrapping a class which does certain scary things to its __dict__ and overrides __reduce__ can cause an error while pickling. An abstract example is given here. (I don't think that supporting this case is a necessity, but it was how I noticed the above issue, and the attached patch fixes both cases.)

Currently the behavior is effectively to just initialize and then pickle the wrapped object. I have and will attach a patch which does that in a more direct way that works in both of the above cases. It also adds a regression test which covers both of the above cases.

Change History (10)

comment:1 Changed 4 years ago by Ben Kraft

Has patch: set
Status: newassigned

Patch is in https://github.com/django/django/pull/5276. I ran utils_tests.test_lazyobject in both python 2 and python 3.

comment:2 Changed 4 years ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Ben Kraft

For what it's worth, I've now realized that you don't actually need to do terrifying things to __dict__ to get the actual error; you just have to keep a reference to a model instance as a property on the instance. I've added an example to the gist here; again Foo here simulates a model. In any case, the fix still works.

comment:4 Changed 4 years ago by Tim Graham

Severity: NormalRelease blocker

This is a regression in 1.8 according to #25426.

comment:5 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Left some small comments for improvement.

comment:6 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

comment:7 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Pending some cosmetic tweaks.

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 35355a4:

Fixed #25389 -- Fixed pickling a SimpleLazyObject wrapping a model.

Pickling a SimpleLazyObject wrapping a model did not work correctly; in
particular it did not add the _django_version attribute added in 42736ac8.
Now it will handle this and other custom __reduce__ methods correctly.

comment:9 Changed 4 years ago by Tim Graham <timograham@…>

In c03f0c2:

[1.8.x] Fixed #25389 -- Fixed pickling a SimpleLazyObject wrapping a model.

Pickling a SimpleLazyObject wrapping a model did not work correctly; in
particular it did not add the _django_version attribute added in 42736ac8.
Now it will handle this and other custom __reduce__ methods correctly.

Backport of 35355a4ffedb2aeed52d5fe3034380ffc6a438db from master

comment:10 Changed 4 years ago by Tim Graham <timograham@…>

In 63a1e912:

[1.9.x] Fixed #25389 -- Fixed pickling a SimpleLazyObject wrapping a model.

Pickling a SimpleLazyObject wrapping a model did not work correctly; in
particular it did not add the _django_version attribute added in 42736ac8.
Now it will handle this and other custom __reduce__ methods correctly.

Backport of 35355a4ffedb2aeed52d5fe3034380ffc6a438db from master

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