New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 24424: migration autodetector won't remove last field on models being deleted #4487

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jwineinger
Contributor

jwineinger commented Apr 13, 2015

Previously the migration auto-detector would produce transition states with invalid tables (for some backends) when removing models with only relation fields. The generate_deleted_models() method would first generate a RemoveField operation for each relation field, and then generate a DeleteModel operation at the end.

In the case where the model being removed only has relation fields, this produces a state where there are no fields on the model, or a table without any columns. This breaks on SQLite (and MySQL, from what I gather).

This patch uses a simple field counter to make sure the last field on a model is not removed before the DeleteModel operation.

jwineinger added some commits Apr 13, 2015

@jwineinger

This comment has been minimized.

Show comment
Hide comment
@jwineinger

jwineinger Apr 13, 2015

Contributor

765dcc0 fixes the test test_m2m_w_through_multistep_remove(self): which was broken by my change, but I think that test needs review. Its name and docstring seem to indicate that it should be testing behavior that produces multiple migrations, but the test itself does not. It asserts that the number of migrations produced is 1, and the operation tests all index into that single migration. I don't yet know enough about the migration system to know whether that test is invalid, but the name/docstring seem inconsistent with what is being tested.

Contributor

jwineinger commented Apr 13, 2015

765dcc0 fixes the test test_m2m_w_through_multistep_remove(self): which was broken by my change, but I think that test needs review. Its name and docstring seem to indicate that it should be testing behavior that produces multiple migrations, but the test itself does not. It asserts that the number of migrations produced is 1, and the operation tests all index into that single migration. I don't yet know enough about the migration system to know whether that test is invalid, but the name/docstring seem inconsistent with what is being tested.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 14, 2015

Member

Do you want to try improving this pull request as suggested by Markus on the ticket?

Member

timgraham commented Apr 14, 2015

Do you want to try improving this pull request as suggested by Markus on the ticket?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 23, 2015

Member

Closing per ticket.

Member

timgraham commented Apr 23, 2015

Closing per ticket.

@timgraham timgraham closed this Apr 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment