Opened 10 years ago

Closed 10 years ago

Last modified 4 years ago

#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 ihulub, 10 years ago

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")

comment:2 by Tim Graham, 10 years ago

Summary: RemoveField irreversible in certain casesAdd the ability to make RemoveField reversible for non-null fields
Triage Stage: UnreviewedAccepted
Type: BugNew 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 Andrew Godwin, 10 years ago

Resolution: wontfix
Status: newclosed

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 Rico Rodriguez, 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 RemoveField.

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.

Version 1, edited 5 years ago by Rico Rodriguez (previous) (next) (diff)

comment:5 by Daniel Smedema, 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 Simon Charette, 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 rm_, 4 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?

Last edited 4 years ago by rm_ (previous) (diff)

comment:8 by rm_, 4 years ago

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