Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#25917 closed Cleanup/optimization (fixed)

Confusing sentence in RemoveField's documentation

Reported by: Baptiste Mispelon Owned by: Kai Feldhoff
Component: Documentation Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The documentation for RemoveField [1] states that:

if the field is not nullable this may make this operation irreversible (apart from any data loss, which of course is irreversible)

I find this sentence confusing.
The note in parentheses implies opposition with the previous statement when it's in fact saying the same thing: the operation is irreversible.

[1] https://docs.djangoproject.com/en/dev/ref/migration-operations/#django.db.migrations.operations.RemoveField

Attachments (1)

25917.diff (924 bytes ) - added by Kai Feldhoff 8 years ago.
Based on e47f0c1 [1.9.x] Fixed #26136

Download all attachments as: .zip

Change History (14)

comment:1 by René Fleschenberg, 8 years ago

As I understand it, the sentence means this: if the field is not nullable, the reverse of RemoveField (adding a field) may fail, because Django does not know which data to add for existing rows.

Apart from that, the forward operation (removing the field) is of course never reversible in so far as it deletes the data in the field.

Question: would RemoveField be reversible if the field has a default value?

comment:2 by Baptiste Mispelon, 8 years ago

If the field is not nullable, is there a case where the reverse of RemoveField would not fail? If not, the use of "may" is confusing.

As for the question about default, that's a good point. I haven't tried and it could make the reverse operation "work" in the sense that Django would be able to get back to a valid state but there's no guarantee that your data would be preserved (any non-default values for that column would be gone).

edit:
I just tried and if your field has a default, then the reverse RemoveField uses that to populate the database (which means it's possible to have a non-nullable field in that case)

Last edited 8 years ago by Baptiste Mispelon (previous) (diff)

comment:3 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Anderson Resende, 8 years ago

Owner: changed from nobody to Anderson Resende
Status: newassigned

comment:5 by Markus Holtermann, 8 years ago

If I see it correctly, unless Django has a value to put into a restored column from a previous migration state (e.g. default, null) RemoveField is irreversible.

comment:6 by Anderson Resende, 8 years ago

Owner: Anderson Resende removed
Status: assignednew

comment:7 by Kai Feldhoff, 8 years ago

It seems like everything is pretty clear here and this ticket could be finished. My suggestion for the respective section in the docs is below. As I am not a native speaker, I appreciate feedback.

By the time everything is fine, I'd like to add this to the docs being my first ticket.

Bear in mind that when reversed this is actually adding a field to a model. To make this possible, Django needs a default value to populate the recreated empty column. This is only possible for a field that is nullable or has a defined default value.
On the other hand if the field is not nullable and does not have a default value, this operation is irreversible (apart from any data loss, which of course is irreversible).

comment:8 by Kai Feldhoff, 8 years ago

Owner: set to Kai Feldhoff
Status: newassigned

by Kai Feldhoff, 8 years ago

Attachment: 25917.diff added

Based on e47f0c1 [1.9.x] Fixed #26136 ...

comment:9 by Kai Feldhoff, 8 years ago

Has patch: set
Needs tests: set

comment:10 by Tim Graham, 8 years ago

Needs tests: unset

Are you able to submit the patch as a pull request? That makes review easier. Please wrap documentation at 79 characters too.

comment:11 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 55481bcd:

Fixed #25917 -- Clarified reversibility of RemoveField.

Thanks kaifeldhoff for the draft patch.

comment:12 by Tim Graham <timograham@…>, 8 years ago

In 6103b499:

[1.9.x] Fixed #25917 -- Clarified reversibility of RemoveField.

Thanks kaifeldhoff for the draft patch.

Backport of 55481bcdeef43ef5e345f8ea3bae87f4a8ec7bb8 from master

comment:13 by Kai Feldhoff, 8 years ago

At least it helped!

I'm having trouble to get Sphinx to work, so I was still unsure about the effect of wrapping the lines.

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