Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25618 closed Bug (fixed)

Django migration system breaks with unhelpful error message if south migrations accidentally retained

Reported by: David Filipovic Owned by: nobody
Component: Migrations Version: 1.7
Severity: Normal Keywords: migrations, south
Cc: django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

For an app that contains both django and south type migrations, the migration system will become unable to perform any migration task for that app, and the error message will not be helpful at all (either "Application labels aren't unique, duplicates: x" - django 1.7, or "table x already exists" - django 1.8). This happens because the app is treated as both migrated and unmigrated at the same time.

To reproduce this error, you can clone the repo: django-south-bug. This repo contains a migrations directory with 2 south type migration in it.

Now run:

python manage.py makemigrations djsouth

followed by:

python manage.py migrate

What is of particular interest here is that the makemigrations command created the initial django migration without deleting the other numbered (south) migrations, thus putting the app into an erroneous state.

Additionally, it is worth noting that in this workflow, step 3 from upgrading-from-south was skipped, so essentially it is user error, however I believe that the error message should not be entirely unhelpful.

The patch attached can be applied to both django 1.7 and 1.8, since they are both affected by this. The patch will break the execution of the migration system at the graph construction phase and report with a helpful error message.

Attachments (2)

django-south-bug-fix.diff (1.1 KB) - added by David Filipovic 4 years ago.
django_style_migrations_fix.diff (1.2 KB) - added by David Filipovic 4 years ago.

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by David Filipovic

Attachment: django-south-bug-fix.diff added

comment:1 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

It's a bit late to add this (in particular, 1.7 is end-of-life Dec. 1 and is only receiving security updates), but adding to 1.8/1.9 seems okay since you offered a patch, so I converted it to a pull request. For 1.10, I proposed to remove detection of south migrations.

comment:2 Changed 4 years ago by Tim Graham <timograham@…>

In c4af8eb:

Refs #25618 -- Removed detection of south migrations in loader.

It doesn't seem relevant for anyone upgrading to Django 1.10
and beyond.

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

Resolution: fixed
Status: newclosed

In 65bff16:

[1.8.x] Fixed #25618 -- Added a helpful error message when Django & south migrations exist in the same directory.

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

In 774a893:

[1.9.x] Fixed #25618 -- Added a helpful error message when Django & south migrations exist in the same directory.

Forwardport of 65bff161ffab1310719bdee495d1e9b35f838c31 from stable/1.8.x

comment:5 Changed 4 years ago by Tim Graham <timograham@…>

In e5ab75d2:

Refs #25618 -- Forwardported 1.8.6 release note.

Forwardport of 65bff161ffab1310719bdee495d1e9b35f838c31 from stable/1.8.x

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

In 9e7d0d9:

[1.9.x] Fixed #25627, refs #25618 -- Removed detection of south migrations in loader.

Backport of c4af8eb366be45420e13a400cc1dcc8fab91c1ad from master

comment:7 Changed 4 years ago by Johannes Hoppe

Resolution: fixed
Status: closednew

The "fix" breaks django_extensions and possibly a lot of other packages that have support for pre django 1.7 versions.

comment:8 Changed 4 years ago by David Filipovic

The problem is actually in the fact that django-extensions is using the migrations module to store south migrations and still claims to support Django 1.4 and later. However, since 1.7 the migrations module was reserved for Django migrations, and even the last official release of South instructs developers to move south migrations to south_migrations module.

So, in conclusion, django-extensions never really fully supported Django 1.7+.

Changed 4 years ago by David Filipovic

comment:9 Changed 4 years ago by David Filipovic

I'm not even sure if this should really be fixed on django's side, but I've created another patch, just in case.

This patch would make it so that in the case such as django-extensions where south migrations module was just left hanging around, against instructions to be moved, Django would just ignore those migrations.

I will create a pull request later, and let the core developers decide whether this should be addressed or not.

comment:10 Changed 4 years ago by Kai Nessig

Cc: django@… added

comment:11 Changed 4 years ago by David Filipovic

@codingjoe, I've created a new pull request to address the problem you reported here.

comment:12 Changed 4 years ago by Nick Smith

FWIW django-extensions has fixed it internally (https://github.com/django-extensions/django-extensions/commit/dbabd69e48e682916e0ffdc7a3567f8fe148ddf3, https://github.com/django-extensions/django-extensions/blob/master/CHANGELOG.md#159), but I acknowledge that Django should expect other third-party apps not to toe the line.

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 8c8a6d8a:

[1.8.x] Fixed #25618 -- Restored migration support for non-upgraded apps.

A non-upgraded app is one that retains South migrations in the
migrations module and doesn't introduce Django migrations.

comment:14 Changed 4 years ago by Tim Graham <timograham@…>

In dbbae2c:

Refs #25618 -- Forwardported 1.8.7 release note.

Forwardport of 8c8a6d8a3f869ecc4d72b96ddb4760a1b59d5e62 from stable/1.8.x

comment:15 Changed 4 years ago by Tim Graham <timograham@…>

In 3dcdfcc8:

[1.9.x] Refs #25618 -- Forwardported 1.8.7 release note.

Forwardport of 8c8a6d8a3f869ecc4d72b96ddb4760a1b59d5e62 from stable/1.8.x

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