Opened 4 years ago

Closed 3 years ago

Last modified 3 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 3 years ago.
Based on e47f0c1 [1.9.x] Fixed #26136

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by René Fleschenberg

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 Changed 4 years ago by Baptiste Mispelon

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 4 years ago by Baptiste Mispelon (previous) (diff)

comment:3 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:4 Changed 4 years ago by Anderson Resende

Owner: changed from nobody to Anderson Resende
Status: newassigned

comment:5 Changed 4 years ago by Markus Holtermann

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 Changed 3 years ago by Anderson Resende

Owner: Anderson Resende deleted
Status: assignednew

comment:7 Changed 3 years ago by Kai Feldhoff

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 Changed 3 years ago by Kai Feldhoff

Owner: set to Kai Feldhoff
Status: newassigned

Changed 3 years ago by Kai Feldhoff

Attachment: 25917.diff added

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

comment:9 Changed 3 years ago by Kai Feldhoff

Has patch: set
Needs tests: set

comment:10 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 55481bcd:

Fixed #25917 -- Clarified reversibility of RemoveField.

Thanks kaifeldhoff for the draft patch.

comment:12 Changed 3 years ago by Tim Graham <timograham@…>

In 6103b499:

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

Thanks kaifeldhoff for the draft patch.

Backport of 55481bcdeef43ef5e345f8ea3bae87f4a8ec7bb8 from master

comment:13 Changed 3 years ago by Kai Feldhoff

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