Opened 18 months ago

Closed 11 months ago

Last modified 10 months ago

#21430 closed New feature (fixed)

Raise Warning when unpickling Models and QuerySet from different version

Reported by: FunkyBob Owned by: anubhav9042
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: loic84 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (15)

comment:1 Changed 18 months ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 follow-up: Changed 18 months ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

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.

comment:3 Changed 17 months ago by prasoon2211

  • Owner changed from nobody to prasoon2211
  • Status changed from new to assigned

comment:4 Changed 17 months ago by prasoon2211

A PR has been filed for this : https://github.com/django/django/pull/2082

comment:5 Changed 11 months ago by anubhav9042

  • Owner changed from prasoon2211 to anubhav9042

Working on this in my GSoC project.

comment:6 Changed 11 months ago by anubhav9042

  • Version changed from 1.6 to master

comment:7 in reply to: ↑ 2 Changed 11 months ago by anubhav9042

Replying to 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 strategy.

Last edited 11 months ago by anubhav9042 (previous) (diff)

comment:8 Changed 11 months ago 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.

comment:9 Changed 11 months ago by anubhav9042

  • Summary changed from Raise explicit error when unpickling QuerySet from incompatible version to Raise explicit error when unpickling Models and QuerySet from incompatible version

comment:10 Changed 11 months ago 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.

comment:11 Changed 11 months ago by loic84

  • Cc loic84 added

comment:12 Changed 11 months ago 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.

comment:13 Changed 11 months ago by anubhav9042

  • Has patch set
  • Summary changed from Raise explicit error when unpickling Models and QuerySet from incompatible version to Raise Warning when unpickling Models and QuerySet from different version

comment:14 Changed 11 months ago by Tim Graham <timograham@…>

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

In 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.

comment:15 Changed 10 months ago by Tim Graham <timograham@…>

In 80f4487d17a0040e9be35e7ee6ac478bafe6504a:

Fixed #22867 -- Memoized django.utils.version.get_git_changeset().

This improves pickling speed in prelease versions of Django; refs #21430.

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