[Django] #23892: Make deconstructible classes forwards compatible
Django
--------------------------------------+-----------------
Reporter: MarkusH | Owner:
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-----------------
The migration operations don't accept any additional arguments (neither
`*args` nor `**kwargs`). This will lead to problems in older (>=1.7)
Django versions if the the migration files for 3rd party apps have been
created with newer Django versions in case the new operations have a
different constructor signature. Thus `**kwargs` should be added to all
operation signatures.
See the discussion on django-developers for details:
https://groups.google.com/forum/#!topic/django-developers/nWHVaG6gK0Y
--
Ticket URL: <https://code.djangoproject.com/ticket/23892>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: MarkusH | Owner: MarkusH
Type: | Status: assigned
Cleanup/optimization | Version: 1.7
Component: Migrations | 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 MarkusH):
* owner: => MarkusH
* needs_better_patch: => 0
* status: new => assigned
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:1>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:2>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
Here's a pull request: https://github.com/django/django/pull/3602
I also touched some validators and the storage backend since they might be
subject of the same problem in the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:3>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:4>
Django
Reporter: MarkusH | Owner: MarkusH
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carl Meyer <carl@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f36151ed169813f2873e13ca9de616cfa4095321"]:
{{{
#!CommitTicketReference repository=""
revision="f36151ed169813f2873e13ca9de616cfa4095321"
Fixed #23892 -- Made deconstructible classes forwards compatible
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:5>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carl Meyer <carl@…>):
In [changeset:"8014001d9287d516c58be80ad71fb63593648b3d"]:
{{{
#!CommitTicketReference repository=""
revision="8014001d9287d516c58be80ad71fb63593648b3d"
[1.7.x] Fixed #23892 -- Made deconstructible classes forwards compatible
Backport of f36151ed169813f2873e13ca9de616cfa4095321 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:6>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I think I committed this over-hastily. Just had a conversation with Shai
in IRC thinking it through, and I think this patch may not have much if
any practical benefit, and may actually cause some harm. Here's the
reasoning:
If someone uses a new 1.8 feature in their migration file, that involves a
new argument to be passed to an operation, chances are good that their
migration actually depends on the correct functioning of that feature. To
take one example, let's say the patch in #23822 is applied and
`CreateModel` now takes a `managers` argument. If someone uses that
feature and writes a migration that depends on a custom manager, having
`CreateModel` accept `**kwargs` in Django 1.7 doesn't actually help them;
it just means that their migration will probably break in some mysterious
way under 1.7 due to lack of the custom manager, rather than breaking in
an obvious way due to the signature mismatch.
Thus, the `**kwargs` forward-compatibility is really only useful if you
imagine a new migrations feature that adds a new argument to an operation,
but where everything degrades gracefully if the new argument is silently
dropped with no effect. In any case (like `managers`) where silently
dropping the arg changes the behavior of the migration file in a
noticeable way, the `**kwargs` forward-compatibility actually likely makes
things harder to debug.
tl;dr - silently dropping arguments you don't understand is usually not a
good plan.
I think that we should roll back these patches, and instead adopt a clear
policy that the migration framework is backwards-compatible to the same
extent as the rest of Django (migration files that worked in Django 1.7
should work the same way in Django 1.8, possibly modulo some deprecation
warning) but, like the rest of Django makes no attempt to be forwards-
compatible (i.e. there is no reason to expect that a migration file
generated in 1.8 should work, or even run, on 1.7). The corollary for
reusable apps supporting multiple Django versions is that they must
generate their migrations on the lowest version of Django they intend to
support. I think this is a reasonable policy, and that trying to keep
migrations both backwards and forwards compatible is a can of worms we
should not open.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:7>
Django
Reporter: MarkusH | Owner: MarkusH
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:8>
Django
Reporter: MarkusH | Owner: MarkusH
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carl Meyer <carl@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2d06c112d1b737578cd54b9193fce20d3bf10a6f"]:
{{{
#!CommitTicketReference repository=""
revision="2d06c112d1b737578cd54b9193fce20d3bf10a6f"
Revert "[1.7.x] Fixed #23892 -- Made deconstructible classes forwards
compatible"
This reverts commit 8014001d9287d516c58be80ad71fb63593648b3d.
Adding kwargs to deconstructed objects does not achieve useful
forward-compatibility in general, since the additional kwargs are silently
dropped rather than having their expected effect. In fact, it can cause
the
failure to be more difficult to debug. Thanks Shai Berger for discussion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:9>
Django
Reporter: MarkusH | Owner: MarkusH
Type: Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carl Meyer <carl@…>):
In [changeset:"bcb693ebd4d3743cb194c6fd05b2d70fb9696a4c"]:
{{{
#!CommitTicketReference repository=""
revision="bcb693ebd4d3743cb194c6fd05b2d70fb9696a4c"
Revert "Fixed #23892 -- Made deconstructible classes forwards compatible"
This reverts commit f36151ed169813f2873e13ca9de616cfa4095321.
Adding kwargs to deconstructed objects does not achieve useful
forward-compatibility in general, since additional arguments are silently
dropped rather than having their intended effect. In fact, it can make the
failure more difficult to diagnose. Thanks Shai Berger for discussion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:10>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: MarkusH
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* status: closed => new
* resolution: fixed =>
* component: Migrations => Documentation
Old description:
> The migration operations don't accept any additional arguments (neither
> `*args` nor `**kwargs`). This will lead to problems in older (>=1.7)
> Django versions if the the migration files for 3rd party apps have been
> created with newer Django versions in case the new operations have a
> different constructor signature. Thus `**kwargs` should be added to all
> operation signatures.
>
> See the discussion on django-developers for details:
> https://groups.google.com/forum/#!topic/django-developers/nWHVaG6gK0Y
New description:
We need to clarify in the documentation whether or not we are attempting
to make migrations forward-compatible (that is, guarantee that it will be
possible to run migrations generated on later Django versions under
earlier Django versions).
Original description:
The migration operations don't accept any additional arguments (neither
`*args` nor `**kwargs`). This will lead to problems in older (>=1.7)
Django versions if the the migration files for 3rd party apps have been
created with newer Django versions in case the new operations have a
different constructor signature. Thus `**kwargs` should be added to all
operation signatures.
See the discussion on django-developers for details:
https://groups.google.com/forum/#!topic/django-developers/nWHVaG6gK0Y
--
Comment:
Rolled back the kwargs addition, at least for now, since there haven't
been any cases presented yet where it would actually offer useful forward-
compatibility.
Re-opening and re-purposing this ticket for discussion and documentation
of the forwards-compatibility policy for migrations. Markus pointed out in
IRC that South effectively maintained forwards-compatibility (that is,
generally it was possible to run migrations generated by a later South
version on an earlier South version - I'm not sure if this was always
true, or just mostly true), so this may be what users expect. I think this
may be true, but it doesn't make maintaining forwards-compatibility any
more feasible for us. Django migrations, because they make use of a
higher-level declarative API in Django (Operations), have much more API
surface than South, and most new features / improvements to migrations
will require non-forwards-compatible changes to the Operations API
(whether that's adding new arguments to existing operations, or new
Operation classes altogether).
So I think our choices boil down to "migrations are feature-frozen" or
"migrations aren't forwards-compatible." Given those choices, I think the
latter is clearly better. A rule that third-party apps should generate
their migrations in the oldest version of Django they support is not
_that_ onerous or hard to understand.
I'll put together a pull request updating the documentation along those
lines. In the meantime, if anyone thinks this is the wrong approach or
I've missed something important, please comment!
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: MarkusH | Owner: MarkusH
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:12>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* cc: info+coding@… (added)
* owner: MarkusH =>
Comment:
I just stumbled over the migration docs for custom fields:
https://docs.djangoproject.com/en/1.7/topics/migrations/#custom-fields .
We tell users to explicitly use `**kwargs` and raise a `TypeError` for
invalid arguments. I'm not sure though if this makes any difference to the
initially described problem I raised here.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:13>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* severity: Normal => Release blocker
Comment:
One problem with using the oldest supported version of Django to generate
your migrations will be autodetector changes in newer versions of Django.
For example, say I am using migrations generated from 1.7 from an app that
supports 1.7 and 1.8. If I am using 1.8, the autodetector will want to
generate new migrations for the new things it supports even if I don't
care about them.
A possible, albeit unfortunate solution would be to ship separate
migrations for each version of Django you want to support.
I think we need to resolve this issue at least by 1.8 final.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:14>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I agree this needs to be resolved by 1.8 final.
I don't think "separate migrations for each supported Django version" is a
reasonable requirement to place on third-party apps.
Tim, do you have any specific examples of changes that have been made to
the autodetector in 1.8 that would cause the problem you mention?
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:15>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
[https://github.com/django/django/pull/3838 PR 3838] might be one (for a
limited set of users and maybe there is no way getting around it).
The managers change was opt-in, and I don't think we've had to rewrite any
migrations included in Django itself besides that which is a good sign.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:16>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
If this is a problem, it may be that we need a setting for "apps to ignore
when running `makemigrations`" (I don't think we should attempt to make
that decision in an automated way). I think we should wait to see whether
in practice that is needed, though. The reason I asked for specific
examples is because I'm having trouble imagining the sort of new feature
that would actually cause this problem. It would have to be something
where an unchanged model now gets a new migration generated under a new
Django version (that adds new in-memory migration state, and thus is
noticed by `makemigrations` if absent).
PR 3838 is unfortunate, but will only impact apps that have an `en-us`
translation which differs from the strings in the code. And the fix is
easy enough - remove that difference.
All in all, none of this changes my conclusion that our recommendation
should be to generate third-party app migrations using the lowest Django
version your app supports.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:17>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I agree we can wait to implement a solution until we see a problem in the
wild. I also agree with your documentation proposal.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:18>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* keywords: => 1.8
* severity: Release blocker => Normal
Comment:
Demoting from release blocker as my initial concerns seem overblown. I'll
tag "1.8" to make sure this is addressed by final though.
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:19>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: 1.8 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* has_patch: 0 => 1
Comment:
Added a PR for the docs clarification:
https://github.com/django/django/pull/4140
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:20>
Django
Reporter: MarkusH | Owner:
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:21>
Django
-------------------------------------+-------------------------------------
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: 1.8 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"e35c70bef44805d47f6a4ae692be878184c4fe1f"]:
{{{
#!CommitTicketReference repository=""
revision="e35c70bef44805d47f6a4ae692be878184c4fe1f"
Fixed #23892 -- Clarified compatibility policy for migrations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: MarkusH | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: 1.8 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a970c277305d7cfa17b290667ce2a145abf96711"]:
{{{
#!CommitTicketReference repository=""
revision="a970c277305d7cfa17b290667ce2a145abf96711"
[1.7.x] Fixed #23892 -- Clarified compatibility policy for migrations.
Backport of e35c70bef44805d47f6a4ae692be878184c4fe1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: MarkusH | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: 1.8 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"e63d9b98e7c5a19e94f45eb85a1f2e53f95a1a7a"]:
{{{
#!CommitTicketReference repository=""
revision="e63d9b98e7c5a19e94f45eb85a1f2e53f95a1a7a"
[1.8.x] Fixed #23892 -- Clarified compatibility policy for migrations.
Backport of e35c70bef44805d47f6a4ae692be878184c4fe1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23892#comment:24>