Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23892 closed Cleanup/optimization (fixed)

Clarify forwards-compatibility policy for migrations

Reported by: Markus Holtermann Owned by: Tim Graham <timograham@…>
Component: Documentation Version: 1.7
Severity: Normal Keywords: 1.8
Cc: info+coding@… 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 (last modified by Carl Meyer)

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

Change History (24)

comment:1 by Markus Holtermann, 10 years ago

Owner: set to Markus Holtermann
Status: newassigned

comment:2 by Carl Meyer, 10 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Markus Holtermann, 10 years ago

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.

comment:4 by Markus Holtermann, 10 years ago

Has patch: set

comment:5 by Carl Meyer <carl@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In f36151ed169813f2873e13ca9de616cfa4095321:

Fixed #23892 -- Made deconstructible classes forwards compatible

comment:6 by Carl Meyer <carl@…>, 10 years ago

In 8014001d9287d516c58be80ad71fb63593648b3d:

[1.7.x] Fixed #23892 -- Made deconstructible classes forwards compatible

Backport of f36151ed169813f2873e13ca9de616cfa4095321 from master.

comment:7 by Carl Meyer, 10 years ago

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.

comment:8 by Carl Meyer, 10 years ago

Resolution: fixed
Status: closednew

comment:9 by Carl Meyer <carl@…>, 10 years ago

Resolution: fixed
Status: newclosed

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

comment:10 by Carl Meyer <carl@…>, 10 years ago

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

comment:11 by Carl Meyer, 10 years ago

Component: MigrationsDocumentation
Description: modified (diff)
Resolution: fixed
Status: closednew
Summary: Make deconstructible classes forwards compatibleClarify forwards-compatibility policy for migrations

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!

comment:12 by Tim Graham, 10 years ago

Has patch: unset

comment:13 by Markus Holtermann, 10 years ago

Cc: info+coding@… added
Owner: Markus Holtermann removed

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.

comment:14 by Tim Graham, 10 years ago

Severity: NormalRelease blocker

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.

comment:15 by Carl Meyer, 10 years ago

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?

comment:16 by Tim Graham, 10 years ago

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.

comment:17 by Carl Meyer, 10 years ago

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.

comment:18 by Tim Graham, 10 years ago

I agree we can wait to implement a solution until we see a problem in the wild. I also agree with your documentation proposal.

comment:19 by Tim Graham, 10 years ago

Keywords: 1.8 added
Severity: Release blockerNormal

Demoting from release blocker as my initial concerns seem overblown. I'll tag "1.8" to make sure this is addressed by final though.

comment:20 by Carl Meyer, 10 years ago

Has patch: set

Added a PR for the docs clarification: https://github.com/django/django/pull/4140

comment:21 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:22 by Tim Graham <timograham@…>, 10 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In e35c70bef44805d47f6a4ae692be878184c4fe1f:

Fixed #23892 -- Clarified compatibility policy for migrations.

comment:23 by Tim Graham <timograham@…>, 10 years ago

In a970c277305d7cfa17b290667ce2a145abf96711:

[1.7.x] Fixed #23892 -- Clarified compatibility policy for migrations.

Backport of e35c70bef44805d47f6a4ae692be878184c4fe1f from master

comment:24 by Tim Graham <timograham@…>, 10 years ago

In e63d9b98e7c5a19e94f45eb85a1f2e53f95a1a7a:

[1.8.x] Fixed #23892 -- Clarified compatibility policy for migrations.

Backport of e35c70bef44805d47f6a4ae692be878184c4fe1f from master

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