Opened 10 years ago
Last modified 10 years ago
#23892 closed Cleanup/optimization
Clarify forwards-compatibility policy for migrations — at Version 11
Reported by: | Markus Holtermann | Owned by: | Markus Holtermann |
---|---|---|---|
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 )
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 (11)
comment:1 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:2 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 10 years ago
comment:4 by , 10 years ago
Has patch: | set |
---|
comment:5 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:7 by , 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 , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
comment:9 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:11 by , 10 years ago
Component: | Migrations → Documentation |
---|---|
Description: | modified (diff) |
Resolution: | fixed |
Status: | closed → new |
Summary: | Make deconstructible classes forwards compatible → Clarify 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!
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.