Fixed #21430 -- Added Exception to be raised when unpickiling QuerySet across unsupported django version #2082

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@prasoon2211

Added an exception UnsupportedUnpickleException that is raised when user tries to unpickle a pickles QuerySet different Django versions.

@prasoon2211 prasoon2211 reopened this Dec 17, 2013

@sdaityari

This comment has been minimized.

Show comment Hide comment
@sdaityari

sdaityari Jan 13, 2014

LGTM

LGTM

@akaariai

View changes

django/db/models/query.py
+ """
+ Allows QuerySet to be unpickled with modification.
+ """
+ if get_version() != dic['django_version']:

This comment has been minimized.

Show comment Hide comment
@akaariai

akaariai Jan 13, 2014

Member

This seems wrong to me - we disallow pickle + unpickle between major versions, but get_version() returns minor version, too. It should be OK to unpickle 1.6.1 querysets in 1.6.2.

@akaariai

akaariai Jan 13, 2014

Member

This seems wrong to me - we disallow pickle + unpickle between major versions, but get_version() returns minor version, too. It should be OK to unpickle 1.6.1 querysets in 1.6.2.

@mjtamlyn

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 13, 2014

Member

As noted on the ticket, please update to also cover Models. Also, it will need documentation (at least release notes, probably also a mention wherever pickling models and querysets is mentioned

Member

mjtamlyn commented Jan 13, 2014

As noted on the ticket, please update to also cover Models. Also, it will need documentation (at least release notes, probably also a mention wherever pickling models and querysets is mentioned

@akaariai

View changes

tests/queryset_pickle/tests.py
+ qs.__class__.__setstate__ = custom_setstate
+ self.assertRaises(UnsupportedUnpickleException,
+ pickle.loads, pickle.dumps(qs))
+ # Restore the class

This comment has been minimized.

Show comment Hide comment
@akaariai

akaariai Jan 13, 2014

Member

You need try-finally here. If the assertRaises fails restore part of the method is skipped.

@akaariai

akaariai Jan 13, 2014

Member

You need try-finally here. If the assertRaises fails restore part of the method is skipped.

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 15, 2014

@akaariai : I've made the changes as you required. Please check if anything else is needed.
@mjtamlyn : Made changes to the docs. Please see this is sufficient.

@akaariai : I've made the changes as you required. Please check if anything else is needed.
@mjtamlyn : Made changes to the docs. Please see this is sufficient.

@mjtamlyn

This comment has been minimized.

Show comment Hide comment
@mjtamlyn

mjtamlyn Jan 15, 2014

Member

Docs look good to me.

Member

mjtamlyn commented Jan 15, 2014

Docs look good to me.

@charettes

View changes

django/db/models/base.py
+ raise UnsupportedUnpickleException("Pickled Model instance's django major "
+ "version %s does not match with current major version %s"
+ %(dic['django_version'], get_major_version()))
+ self.__dict__ = dic

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 17, 2014

Member

What about using state instead of dic here?

@charettes

charettes Jan 17, 2014

Member

What about using state instead of dic here?

@charettes

View changes

django/db/models/query.py
+ raise UnsupportedUnpickleException("Pickled QuerySet's django major version "
+ "%s does not match with current major version %s"
+ %(dic['django_version'], get_major_version()))
+ self.__dict__ = dic

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 17, 2014

Member

Idem.

@charettes

View changes

django/utils/version.py
+
+ parts = 2 if version[2] == 0 else 3
+ main = '.'.join(str(x) for x in version[:parts])
+ return main

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 17, 2014

Member

In order to keep the code DRY you should call this method from get_version and reuse it's main component.

Also I'm not sure about the naming here -- per PEP386 I think this component should be name major and not main. Could you rename it while you're at it?

@charettes

charettes Jan 17, 2014

Member

In order to keep the code DRY you should call this method from get_version and reuse it's main component.

Also I'm not sure about the naming here -- per PEP386 I think this component should be name major and not main. Could you rename it while you're at it?

@charettes

View changes

tests/model_package/tests.py
+ pickle.loads, pickle.dumps(p))
+ finally:
+ # Restore the class
+ Publication.__setstate__ = original_setstate

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 17, 2014

Member

What about creating a proxy subclass of Publication with an overridden __reduce__ method returning the previous django_version instead.

class PreviousDjangoVersionPublication(Publication):
    class Meta:
        proxy = True

    def __reduce__(self):
        model_unpickle, (class_id, defers, deferred_class_factory), data = super(
            PreviousDjangoVersionPublication, self
        ).__reduce__()
        data['django_version'] = float(get_major_version()) - 0.1 # previous major version
        return model_unpickle, (class_id, defers, deferred_class_factory), data

With this class defined the test could be simplified greatly and avoid monkey patching.

@charettes

charettes Jan 17, 2014

Member

What about creating a proxy subclass of Publication with an overridden __reduce__ method returning the previous django_version instead.

class PreviousDjangoVersionPublication(Publication):
    class Meta:
        proxy = True

    def __reduce__(self):
        model_unpickle, (class_id, defers, deferred_class_factory), data = super(
            PreviousDjangoVersionPublication, self
        ).__reduce__()
        data['django_version'] = float(get_major_version()) - 0.1 # previous major version
        return model_unpickle, (class_id, defers, deferred_class_factory), data

With this class defined the test could be simplified greatly and avoid monkey patching.

@charettes

View changes

tests/queryset_pickle/tests.py
+ finally:
+ # Restore the class
+ qs.__class__.__getstate__ = original_getstate
+ qs.__class__.__setstate__ = original_setstate

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 17, 2014

Member

Idem here, define a QuerySet subclass and override __getstate__ to return the previous 'django_version' and define a manager on Group -- say previous_version_objects -- using PreviousVersionQuerySet.as_manager().

@charettes

charettes Jan 17, 2014

Member

Idem here, define a QuerySet subclass and override __getstate__ to return the previous 'django_version' and define a manager on Group -- say previous_version_objects -- using PreviousVersionQuerySet.as_manager().

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 18, 2014

@charettes : I've made the changes you required. Tell me if more is needed.

@charettes : I've made the changes you required. Tell me if more is needed.

@charettes

View changes

tests/queryset_pickle/models.py
+ raise UnsupportedUnpickleException("Pickled QuerySet's django version "
+ "%s does not match with current version %s"
+ %(state['django_version'], version))
+ self.__dict__ = state

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

You're not testing models.QuerySet.__setstate__ at all here.

Override __getstate__ instead:

class PreviousDjangoVersionQuerySet(models.QuerySet):
    def __getstate__(self):
        state = super(models.QuerySet, self).__getstate__()
        state['django_version'] = str(float(state['django_version']) - 1)  # previous major version
        return state
@charettes

charettes Jan 18, 2014

Member

You're not testing models.QuerySet.__setstate__ at all here.

Override __getstate__ instead:

class PreviousDjangoVersionQuerySet(models.QuerySet):
    def __getstate__(self):
        state = super(models.QuerySet, self).__getstate__()
        state['django_version'] = str(float(state['django_version']) - 1)  # previous major version
        return state
@charettes

View changes

tests/queryset_pickle/tests.py
@@ -6,7 +6,7 @@
from django.test import TestCase
from .models import Group, Event, Happening, Container, M2MModel
-
+from django.db.models.query_utils import UnsupportedUnpickleException

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

Group and alphabetize this import.

@charettes

charettes Jan 18, 2014

Member

Group and alphabetize this import.

@charettes

View changes

tests/queryset_pickle/tests.py
+ # These need to be updated in accordance with the changes in the QuerySet class
+
+ # NOTE: The Group object has a custom Manager defined in models.py
+ g = Group.objects.create(name='foo')

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

You shouldn't need to create a group object.

@charettes

charettes Jan 18, 2014

Member

You shouldn't need to create a group object.

@charettes

View changes

tests/queryset_pickle/models.py
@@ -2,6 +2,8 @@
from django.db import models
from django.utils.translation import ugettext_lazy as _
+from django.utils.version import get_major_version
+from django.db.models.query_utils import UnsupportedUnpickleException

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

Alphabetize import.

@charettes

charettes Jan 18, 2014

Member

Alphabetize import.

@charettes

View changes

tests/model_package/tests.py
from .models.publication import Publication
from .models.article import Article
+import pickle

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

Move this stdlib import before Django's one.

@charettes

charettes Jan 18, 2014

Member

Move this stdlib import before Django's one.

@charettes

View changes

tests/model_package/tests.py
+ raise UnsupportedUnpickleException("Pickled Model instance's django major "
+ "version %s does not match with current version %s"
+ %(state['django_version'], version))
+ self.__dict__ = state

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

You're not testing __setstate__ here.

Just define a models.Model subclass with an overridden __reduce__ method that alters the data['django_version'] to be of the previous major version.

@charettes

charettes Jan 18, 2014

Member

You're not testing __setstate__ here.

Just define a models.Model subclass with an overridden __reduce__ method that alters the data['django_version'] to be of the previous major version.

@charettes

View changes

django/db/models/query_utils.py
+ """
+ Tried to unpickle a QuerySet from incompatible version of django.
+ """
+ pass

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

Not sure about the naming of this exception nor if it should be defined here since it's both used by models.Model and query.QuerySet.

@charettes

charettes Jan 18, 2014

Member

Not sure about the naming of this exception nor if it should be defined here since it's both used by models.Model and query.QuerySet.

@charettes

View changes

django/db/models/base.py
@@ -508,6 +511,13 @@ def __reduce__(self):
class_id = model._meta.app_label, model._meta.object_name
return (model_unpickle, (class_id, defers, deferred_class_factory), data)
+ def __setstate__(self, state):
+ if get_major_version() != state['django_version']:

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

This will raise a KeyError with objects pickled in Django 1.6 since their state didn't include a 'django_version'. You should use state.get('django_version') instead and add an additional test for this case.

@charettes

charettes Jan 18, 2014

Member

This will raise a KeyError with objects pickled in Django 1.6 since their state didn't include a 'django_version'. You should use state.get('django_version') instead and add an additional test for this case.

@charettes

View changes

django/db/models/query.py
+ """
+ Allows QuerySet to be unpickled with modification.
+ """
+ if get_major_version() != state['django_version']:

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 18, 2014

Member

state.get('django_version') should also be used here and an another test should be added to the suite.

@charettes

charettes Jan 18, 2014

Member

state.get('django_version') should also be used here and an another test should be added to the suite.

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 19, 2014

@charettes : The latest commit contains all changes, I think. Also, for the exception name, I've decided to drop the Unsupported part. It's now just UnpickleException. Also, there's a new file, django/db/models/common_utils.py in which I've moved the Exception definition. Let me know of any more changes.

@charettes : The latest commit contains all changes, I think. Also, for the exception name, I've decided to drop the Unsupported part. It's now just UnpickleException. Also, there's a new file, django/db/models/common_utils.py in which I've moved the Exception definition. Let me know of any more changes.

@charettes

View changes

tests/queryset_pickle/tests.py
+ # if key not found in state, then test will not pass
+ qs = Group.objects.all()
+ state = qs.__getstate__()
+ self.assertEqual(state['django_version'], get_major_version())

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

What you want to test instead is the unpickling of queryset pickled in Django < 1.7. You can achieve this by declaring a subclass of models.QuerySet that deletes its django_version state key:

class MissingDjangoVersionQuerySet(models.QuerySet):
    def __getstate__(self):
        state = super(MissingDjangoVersionQuerySet, self).__getstate__()
        del state['django_version']
        return state

# Add a `missing_django_version_objects = MissingDjangoVersionQuerySet.as_manager()` to `Group`.

Then you want to make sure __setstate__ handles this gracefully, that is raising an UnpickleException and not a KeyError.

@charettes

charettes Jan 19, 2014

Member

What you want to test instead is the unpickling of queryset pickled in Django < 1.7. You can achieve this by declaring a subclass of models.QuerySet that deletes its django_version state key:

class MissingDjangoVersionQuerySet(models.QuerySet):
    def __getstate__(self):
        state = super(MissingDjangoVersionQuerySet, self).__getstate__()
        del state['django_version']
        return state

# Add a `missing_django_version_objects = MissingDjangoVersionQuerySet.as_manager()` to `Group`.

Then you want to make sure __setstate__ handles this gracefully, that is raising an UnpickleException and not a KeyError.

@charettes

View changes

tests/queryset_pickle/tests.py
@@ -105,3 +107,21 @@ def test_pickle_prefetch_related_idempotence(self):
# Second pickling
groups = pickle.loads(pickle.dumps(groups))
self.assertQuerysetEqual(groups, [g], lambda x: x)
+
+ def test_queryset_setstate_dj_version(self):

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

test_missing_django_version_unpickling

@charettes

charettes Jan 19, 2014

Member

test_missing_django_version_unpickling

@charettes

View changes

tests/queryset_pickle/tests.py
+ state = qs.__getstate__()
+ self.assertEqual(state['django_version'], get_major_version())
+
+ def test_unsupported_unpickle_exception(self):

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

test_previous_django_version_unpickling

@charettes

charettes Jan 19, 2014

Member

test_previous_django_version_unpickling

@charettes

View changes

tests/queryset_pickle/tests.py
+ # So, we will create a custom getstate and setstate method
+ # These need to be updated in accordance with the changes in the QuerySet class
+
+ # NOTE: The Group object has a custom Manager defined in models.py

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

The sole reference to ticket #21430 should be enough.

@charettes

charettes Jan 19, 2014

Member

The sole reference to ticket #21430 should be enough.

@charettes

View changes

tests/model_package/tests.py
+ data = reduce_list[-1]
+ data['django_version'] = str(float(get_major_version()) - 0.1)
+ reduce_list[-1] = data
+ return tuple(reduce_list)

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

You don't need to turn __reduce__ return value to a list and then back to a tuple nor reassign data to reduce_list[-1] since you already have a reference.

@charettes

charettes Jan 19, 2014

Member

You don't need to turn __reduce__ return value to a list and then back to a tuple nor reassign data to reduce_list[-1] since you already have a reference.

@charettes

View changes

tests/model_package/tests.py
+ def test_unsupported_unpickle_exception(self):
+ # ticket 21430
+ # The __reduce__ function will set the dic['django_version'] to current major version.
+ # So, we reduce the version number to the last major version to test this exception.

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

The sole reference to ticket #21430 should be enough.

@charettes

charettes Jan 19, 2014

Member

The sole reference to ticket #21430 should be enough.

@charettes

View changes

django/db/models/common_utils.py
+ """
+ Tried to unpickle a QuerySet from incompatible version of django.
+ """
+ pass

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 19, 2014

Member

I think it belongs in django.core.exception with an adjusted __doc__.

@charettes

charettes Jan 19, 2014

Member

I think it belongs in django.core.exception with an adjusted __doc__.

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 20, 2014

@charettes : Now?

@charettes

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 20, 2014

Member

Thanks for all the changes this is looking good. Many changes have landed in master recently and your change don't apply anymore, could you rebase on top of master?

Member

charettes commented Jan 20, 2014

Thanks for all the changes this is looking good. Many changes have landed in master recently and your change don't apply anymore, could you rebase on top of master?

Fixed #21430 -- Added Exception to be raised when unpickiling QuerySet
across unsupported django version.

Added exception (UnpickleException).
Added tests for QuerySets.
Added tests for Models.

Added docs for UnpickleException. Additions done under:
* release notes for 1.7
* queryset reference docs

See PR #2082 for relevant discussion.
@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 21, 2014

@charettes : Okay then. I've squashed all commits into a single commit and rebased on top of master.

@charettes : Okay then. I've squashed all commits into a single commit and rebased on top of master.

@akaariai

This comment has been minimized.

Show comment Hide comment
@akaariai

akaariai Jan 21, 2014

Member

On quick glance looks good to me.

We might want to relax the version check for models if it happens that for example 1.6 models are unpickleable in 1.7. But it is best to default to error and then add exceptions to the check if unpickle is possible.

For models it might even make sense to add unpickle conversions between major versions if there are incompatibilities. If we know the pickled model's version it should be possible to convert internal data-structures between versions.

For QuerySets it seems unlikely that unpickling across major versions will ever be possible. The data-structures and possible interactions are just too complex.

Member

akaariai commented Jan 21, 2014

On quick glance looks good to me.

We might want to relax the version check for models if it happens that for example 1.6 models are unpickleable in 1.7. But it is best to default to error and then add exceptions to the check if unpickle is possible.

For models it might even make sense to add unpickle conversions between major versions if there are incompatibilities. If we know the pickled model's version it should be possible to convert internal data-structures between versions.

For QuerySets it seems unlikely that unpickling across major versions will ever be possible. The data-structures and possible interactions are just too complex.

django/db/models/query.py
@@ -7,7 +7,7 @@
import sys
from django.conf import settings
-from django.core import exceptions
+from django.core.exceptions import UnpickleException

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 22, 2014

Member

exceptions.ObjectDoesNotExist is referenced at line 1819.

@charettes

charettes Jan 22, 2014

Member

exceptions.ObjectDoesNotExist is referenced at line 1819.

@charettes

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Jan 22, 2014

Member

Also seems to cause a regression:

FAIL: test_regression_7314_7372 (extra_regress.tests.ExtraRegressTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/simon/workspace/django/tests/extra_regress/tests.py", line 37, in test_regression_7314_7372
    self.assertEqual(rm2.base.title, 'First Revision')
AssertionError: u'Second Revision' != u'First Revision'
- Second Revision
+ First Revision

@akaariai does shipping the strict model version checks as is makes sense to you or should we stick to queryset for now?

Member

charettes commented Jan 22, 2014

Also seems to cause a regression:

FAIL: test_regression_7314_7372 (extra_regress.tests.ExtraRegressTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/simon/workspace/django/tests/extra_regress/tests.py", line 37, in test_regression_7314_7372
    self.assertEqual(rm2.base.title, 'First Revision')
AssertionError: u'Second Revision' != u'First Revision'
- Second Revision
+ First Revision

@akaariai does shipping the strict model version checks as is makes sense to you or should we stick to queryset for now?

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jan 24, 2014

So it seems there are test failures. I'll see what I can do and comment back in 4-5 days. Also, I'll probably be away from internet connectivity over the next 2-3 days so, please don't except a reply to any comments made till them. Thanks!

So it seems there are test failures. I'll see what I can do and comment back in 4-5 days. Also, I'll probably be away from internet connectivity over the next 2-3 days so, please don't except a reply to any comments made till them. Thanks!

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Jun 1, 2014

Owner

@prasoon2211 - are you interested in trying to complete this? If not, we have a GSoC student interested in finishing it up. Please let us know, thanks!

Owner

timgraham commented Jun 1, 2014

@prasoon2211 - are you interested in trying to complete this? If not, we have a GSoC student interested in finishing it up. Please let us know, thanks!

@prasoon2211

This comment has been minimized.

Show comment Hide comment
@prasoon2211

prasoon2211 Jun 1, 2014

@timgraham: I had totally forgotten about this patch! Sorry for the delay. Anyway, I have my own GSoC project to deal with at the moment, so no. Anyone is welcome to finish this!
IIRC, this patch was done but there were some test failures which'll need to be looked at.

@timgraham: I had totally forgotten about this patch! Sorry for the delay. Anyway, I have my own GSoC project to deal with at the moment, so no. Anyone is welcome to finish this!
IIRC, this patch was done but there were some test failures which'll need to be looked at.

@coder9042

This comment has been minimized.

Show comment Hide comment
@coder9042

coder9042 Jun 2, 2014

Contributor

@prasoon2211
I am happy to complete it.

Contributor

coder9042 commented Jun 2, 2014

@prasoon2211
I am happy to complete it.

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Jun 2, 2014

Owner

Closing this since it'll be updated separately.

Owner

timgraham commented Jun 2, 2014

Closing this since it'll be updated separately.

@timgraham timgraham closed this Jun 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment