[Django] #21430: Raise explicit error when unpickling QuerySet from incompatible version
Django
----------------------------------------------+--------------------
Reporter: FunkyBob | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Currently the errors from doing this are not obvious at all, and make
debugging a nightmare.
Having a specific exception to look for in such cases would greatly
simplify everyones job.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: FunkyBob | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I think this is a good idea. We should invent a new exception type
UnsupportedUnpickleException or something like that.
The implementation doesn't need to be more complicated than adding version
= django's major version to the pickled state in `__getstate__` and then
checking that the version is compatible in `__setstate__`. For first try
we should just raise error if versions differ, later on we might want to
try to actually recover from pickling errors. Unfortunately Query and all
that it depends on seems to be way too complex to successfully do cross-
version unpickling.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:1>
Django
Reporter: FunkyBob | Owner: nobody
Type: New feature | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* stage: Unreviewed => Accepted
Comment:
By the way, I don't think we should leave this to just QuerySet. I know
that at least Model unpickling can have this same issue. But for model
unpickling we might be able to actually do cross-version conversions in
`__getstate__()` and `__setstate__()`. Again, first try lets just throw an
error.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:2>
Django
Reporter: FunkyBob | Owner:
Type: New feature | prasoon2211
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prasoon2211):
* status: new => assigned
* owner: nobody => prasoon2211
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:3>
Django
Reporter: FunkyBob | Owner:
Type: New feature | prasoon2211
Component: Database layer | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prasoon2211):
A PR has been filed for this : https://github.com/django/django/pull/2082
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:4>
Django
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* owner: prasoon2211 => anubhav9042
Comment:
Working on this in my GSoC project.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:5>
Django
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* version: 1.6 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:6>
Django
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
Replying to [comment:2 akaariai]:
> By the way, I don't think we should leave this to just QuerySet. I know
that at least Model unpickling can have this same issue. But for model
unpickling we might be able to actually do cross-version conversions in
`__getstate__()` and `__setstate__()`. Again, first try lets just throw an
error.
Can you please comment out your thoughts regarding cross-version
conversion. I mean if you have thought of any way to pickle `Models` than
the normal way or some other startegy.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:7>
Django
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
First we will deal with raising errors. Cross-version conversions in
`Model` would be dealt with separately.
Regarding raising errors, we have reached where we are raising errors when
version is different. Any ideas on how we can proceed further and check if
versions are compatible or not.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:8>
Django
version
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:9>
Django
version
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
We discussed this on IRC with Anubhav and Simon.
My understanding of the issue is that we can't detect if a pickle is
really incompatible since unpickling gives us silently corrupted objects
rather than hard failures.
The proposed patch
(https://github.com/coder9042/django/compare/gsoc_21430) raises an
exception every time a pickle from another major version is detected,
based on the premise that we "try" not to break pickle in minor version.
While it's true that minor releases are less likely to break pickling, a
security issue or a data-loss issue could easily require a change in the
ORM that would break pickling.
I proposed we used runtime warnings rather than exceptions when **any**
version mismatch is detected, these can easily be ignored if you've never
cared/encountered the problem, but when you are affected, you'll clearly
get a pointer to the issue. Another option is to mix the two approaches,
forbid pickles across major versions with exceptions, and only warn about
pickles across minor version.
Our "release checklist" documentation could also add a recommendation of
clearing pickled objects when upgrading Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:10>
Django
version
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* cc: loic84 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:11>
Django
version
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
RuntimeWarnings are OK to me. The idea is to make problems in this area
easier to detect, and warnings will suffice for that.
As for making model pickling compatible between major versions, lets split
that issue into separate ticket.
So, the proposed plan for this ticket is: raise RuntimeWarning on every
detected major or minor version change for both models and querysets.
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: assigned
(models, ORM) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anubhav9042):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/2766
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
(models, ORM) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"42736ac8e8c31137131714013951249a09e6e7d4"]:
{{{
#!CommitTicketReference repository=""
revision="42736ac8e8c31137131714013951249a09e6e7d4"
Fixed #21430 -- Added a RuntimeWarning when unpickling Models and
QuerySets from a different Django version.
Thanks FunkyBob for the suggestion, prasoon2211 for the initial patch,
and akaariai, loic, and charettes for helping in shaping the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: FunkyBob | Owner:
Type: New feature | anubhav9042
Component: Database layer | Status: closed
(models, ORM) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"80f4487d17a0040e9be35e7ee6ac478bafe6504a"]:
{{{
#!CommitTicketReference repository=""
revision="80f4487d17a0040e9be35e7ee6ac478bafe6504a"
Fixed #22867 -- Memoized django.utils.version.get_git_changeset().
This improves pickling speed in prelease versions of Django; refs #21430.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21430#comment:15>