#23048 closed New feature (wontfix)
Add the ability to make RemoveField reversible for non-null fields
| Reported by: | Harris Lapiroff | Owned by: | nobody |
|---|---|---|---|
| Component: | Migrations | Version: | 1.7-rc-1 |
| Severity: | Normal | Keywords: | |
| Cc: | rm_ | Triage Stage: | Accepted |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
RemoveField is an irreversible operation if a field's original state requires it be non-null.
To reproduce:
- Write a model with a field with
null=False - Make and run initial migrations.
- Create and save some instances of your model.
- Remove the field.
- Make and run the migration.
- Attempt to migrate back to the initial migration (i.e.,
python manage.py migrate myapp 0001)
You'll get django.db.utils.IntegrityError: myapp_mymodel__new.field may not be NULL
This behavior is documented (https://docs.djangoproject.com/en/dev/ref/migration-operations/#django.db.migrations.operations.RemoveField), but I believe there should be a way to provide a default value for the field when unapplying the migration.
Change History (8)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
| Summary: | RemoveField irreversible in certain cases → Add the ability to make RemoveField reversible for non-null fields |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Type: | Bug → New feature |
I don't think adding a default on the model field would make any difference. Anyway, it seems like a reasonable feature request although I'm not sure how feasible it will be; tentatively accepting, although someone else can feel free to close with a rationale of why we can't do it.
comment:3 by , 10 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
Adding a default on the model field or on the field in the migration should solve this, as it'll be able to populate the new field with that when it makes it. The only other option is to prompt people for a default every time they *remove* a not null field, which really confused people in South, so let's not do that.
Closing as WONTFIX, as the solution is a simple edit to the migration.
comment:4 by , 5 years ago
Why was this closed as wontfix?
The provided solutions from the last couple of comments don't address the fact that developers often take over projects that don't belong to them initially; I can't go back in time and ask the developer to put defaults on a NOT NULL field. Furthermore, solutions like inserting AlterField in the migration itself clearly don't help when the migration breaks precisely at the (preceding) RemoveField step.
There is a completely reasonable fix to this and it's to add something like a "reverse override" object in the migrations.RemoveField constructor that allows you to change the field-adding behavior of the reversed query. Something like
Code highlighting:
migrations.RemoveField(model_name="child", name="age_at_birth", reversal_options={"null": True}),
Is a clean enough API and provides the necessary state to go backwards and forwards cleanly.
comment:5 by , 5 years ago
It seems like the clearest interface and most general solution would be to allow manually specifying the reverse operation as an entire object. With preserve_default=False you can then set a default on a field that was nullable. For example:
# migration 0003
...
operations = [
migrations.AddField(
model_name='invitation',
name='first_name',
field=models.TextField(default=''),
preserve_default=False,
),
]
...
# migration 0004
...
operations = [
migrations.RemoveField(
model_name='invitation',
name='first_name',
reverse_operation=migrations.AddField( # <-- this is what I am proposing
...
field=models.TextField(default=''),
preserve_default=False,
...
),
),
...
This could be applied to AlterField as well I would think.
comment:6 by , 5 years ago
@Rico
I can't go back in time and ask the developer to put defaults on a NOT NULL field.
You simply have to edit the migration that added the field and set a default and preserve_default=False to the AddField or CreateModel operation.
@Daniel
If default is defined on the field like in your example (migration 0003) the migration framework will already knows that it must recreate the column with the provided default on the RemoveField reversal. I'm not sure the added complexity of reverse_operation is worth it.
Maybe we could raise a warning/notice to users when generating a migration with a RemoveField for a field that doesn't define a default to suggest editing the migration that introduced the field to add a default?
comment:7 by , 5 years ago
Found this debugging a similar issue. My problem comes from having some migrations tests that test a specific migrations for a field that I'm trying to remove. So in my migrations history I have: addfield -> make nullable -> remove field
I've tried to add a default as suggested above:
- field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='translatable_messages.Language'),
+ field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='translatable_messages.Language', default=1),
+ preserve_default=False
but instead of postgres warning about the field having null values when it's not supposed to i get this:
django.db.utils.OperationalError: cannot ALTER TABLE "translatable_messages_translatablemessage" because it has pending trigger events
I'm tempted to remove the tests on the migrations and call it a day but maybe there's something else I can do?
comment:8 by , 5 years ago
| Cc: | added |
|---|
I believe there is a way to provide a default value for the field.
Have you tried passing the "default=xyz" parameter to the model field?
Example:
name = model.CharField(null=False, default="noname")