#31503 closed Bug (fixed)
Moving a unique constraint from unique_together to Field.unique generate an invalid migration.
Reported by: | Xiang Wang | Owned by: | David Wobrock |
---|---|---|---|
Component: | Migrations | Version: | 3.0 |
Severity: | Normal | Keywords: | unique_together unique migrations |
Cc: | ramwin@…, David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
You can see a demo example to show the bug at [github](https://github.com/ramwin/testunique/).
I met a problem when I convert a unique_together
to the unique=True
attribute.
first commit, everything is ok
I create a model file.
// testapp/models.py class MyModel(models.Model): name = models.CharField(max_length=32) class Meta: unique_together = ("name", )
the migrations file looks like this.
// testapp/migrations/0001_initial.py # Generated by Django 3.0.5 on 2020-04-22 12:47 from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ ('testapp', '0001_initial'), ] operations = [ migrations.AlterField( model_name='mymodel', name='name', field=models.CharField(max_length=32, unique=True), ), migrations.AlterUniqueTogether( name='mymodel', unique_together=set(), ), ]
second commit: then I remove the unique_together and add unique=True to the field MyModel.name
model file class MyModel(models.Model): name = models.CharField(max_length=32, unique=True) class Meta: pass # unique_together = ("name", ) 0002 migrations file class Migration(migrations.Migration): dependencies = [ ('testapp', '0001_initial'), ] operations = [ migrations.AlterField( model_name='mymodel', name='name', field=models.CharField(max_length=32, unique=True), ), migrations.AlterUniqueTogether( name='mymodel', unique_together=set(), ), ]
However, when I apply the migrations, an error occurs;
wangx@aliyun:~/testunique$ python3 manage.py migrate Operations to perform: Apply all migrations: admin, auth, contenttypes, sessions, testapp Running migrations: Applying contenttypes.0001_initial... OK Applying auth.0001_initial... OK Applying admin.0001_initial... OK Applying admin.0002_logentry_remove_auto_add... OK Applying admin.0003_logentry_add_action_flag_choices... OK Applying contenttypes.0002_remove_content_type_name... OK Applying auth.0002_alter_permission_name_max_length... OK Applying auth.0003_alter_user_email_max_length... OK Applying auth.0004_alter_user_username_opts... OK Applying auth.0005_alter_user_last_login_null... OK Applying auth.0006_require_contenttypes_0002... OK Applying auth.0007_alter_validators_add_error_messages... OK Applying auth.0008_alter_user_username_max_length... OK Applying auth.0009_alter_user_last_name_max_length... OK Applying auth.0010_alter_group_name_max_length... OK Applying auth.0011_update_proxy_permissions... OK Applying sessions.0001_initial... OK Applying testapp.0001_initial... OK Applying testapp.0002_auto_20200422_1247...Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/mysql/base.py", line 74, in execute return self.cursor.execute(query, args) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 209, in execute res = self._query(query) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 315, in _query db.query(q) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/connections.py", line 239, in query _mysql.connection.query(self, query) MySQLdb._exceptions.OperationalError: (1061, "Duplicate key name 'testapp_mymodel_name_ba5e2bd2_uniq'") The above exception was the direct cause of the following exception: Traceback (most recent call last): File "manage.py", line 21, in <module> main() File "manage.py", line 17, in main execute_from_command_line(sys.argv) File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 401, in execute_from_command_line utility.execute() File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 395, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 328, in run_from_argv self.execute(*args, **cmd_options) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 369, in execute output = self.handle(*args, **options) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 83, in wrapped res = handle_func(*args, **kwargs) File "/usr/local/lib/python3.6/dist-packages/django/core/management/commands/migrate.py", line 233, in handle fake_initial=fake_initial, File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 117, in migrate state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 147, in _migrate_all_forwards state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 245, in apply_migration state = migration.apply(state, schema_editor) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/migration.py", line 124, in apply operation.database_forwards(self.app_label, schema_editor, old_state, project_state) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/operations/fields.py", line 249, in database_forwards schema_editor.alter_field(from_model, from_field, to_field) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 565, in alter_field old_db_params, new_db_params, strict) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 745, in _alter_field self.execute(self._create_unique_sql(model, [new_field.column])) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 142, in execute cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 100, in execute return super().execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 68, in execute return self._execute_with_wrappers(sql, params, many=False, executor=self._execute) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 77, in _execute_with_wrappers return executor(sql, params, many, context) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/utils.py", line 90, in __exit__ raise dj_exc_value.with_traceback(traceback) from exc_value File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/mysql/base.py", line 74, in execute return self.cursor.execute(query, args) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 209, in execute res = self._query(query) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 315, in _query db.query(q) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/connections.py", line 239, in query _mysql.connection.query(self, query) django.db.utils.OperationalError: (1061, "Duplicate key name 'testapp_mymodel_name_ba5e2bd2_uniq'")
I check the sql for these migrations, it shows:
wangx@aliyun:~/testunique$ python3 manage.py sqlmigrate testapp 0001 -- -- Create model MyModel -- CREATE TABLE `testapp_mymodel` (`id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY, `name` varchar(32) NOT NULL); ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq` UNIQUE (`name`); wangx@aliyun:~/testunique$ python3 manage.py sqlmigrate testapp 0002 -- -- Alter field name on mymodel -- ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq` UNIQUE (`name`); -- -- Alter unique_together for mymodel (0 constraint(s)) -- ALTER TABLE `testapp_mymodel` DROP INDEX `testapp_mymodel_name_ba5e2bd2_uniq`;
it looks like django will
- first create the index for unique=True
- second drop the index for unique_together=('name', )
but the program for creating index name generates the same index name :testapp_mymodel_name_ba5e2bd2_uniq, so when django create the same index, Duplicate key name error occurs.
Attachments (1)
Change History (12)
by , 5 years ago
Attachment: | testunique.tar.gz added |
---|
comment:1 by , 5 years ago
Summary: | I met a problem when I convert a unique_together to the unique=True attribute. → Moving a unique constraint from unique_together to Field.unique generate an invalid migration. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Thank your for your report.
I guess this is a bug in the auto-detector where the AlterUniqueTogether
should appear before the AlterField
that adds the Field.unique=True
. I assume this is the case because generate_altered_unique_together
is run after generate_altered_fields
and the former doesn't add any dependencies on ensure operations are properly re-ordered.
Not sure it's worth adjusting AddConstraint
ordering as well since the chance of adding a UniqueConstraint
with a colliding .name
are really slim.
Xiang, can you confirm that re-ordering your operations so that AlterUniqueTogether
is performed before AlterField
addresses your issue?
comment:2 by , 5 years ago
I think changing the format of index name if the CONSTRAINT is create by the unique_together will work either.
e.g.:
ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq_by_unique_together` UNIQUE (`name`); ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq_by_field` UNIQUE (`name`);
I tried the sql in reverse order. It works.
wangx@aliyun:~/testunique$ python3 manage.py migrate testapp 0002 Operations to perform: Target specific migration: 0002_auto_20200422_1247, from testapp Running migrations: Applying testapp.0002_auto_20200422_1247...Traceback (most recent call last): File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/mysql/base.py", line 74, in execute return self.cursor.execute(query, args) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 209, in execute res = self._query(query) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 315, in _query db.query(q) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/connections.py", line 239, in query _mysql.connection.query(self, query) MySQLdb._exceptions.OperationalError: (1061, "Duplicate key name 'testapp_mymodel_name_ba5e2bd2_uniq'") The above exception was the direct cause of the following exception: Traceback (most recent call last): File "manage.py", line 21, in <module> main() File "manage.py", line 17, in main execute_from_command_line(sys.argv) File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 401, in execute_from_command_line utility.execute() File "/usr/local/lib/python3.6/dist-packages/django/core/management/__init__.py", line 395, in execute self.fetch_command(subcommand).run_from_argv(self.argv) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 328, in run_from_argv self.execute(*args, **cmd_options) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 369, in execute output = self.handle(*args, **options) File "/usr/local/lib/python3.6/dist-packages/django/core/management/base.py", line 83, in wrapped res = handle_func(*args, **kwargs) File "/usr/local/lib/python3.6/dist-packages/django/core/management/commands/migrate.py", line 233, in handle fake_initial=fake_initial, File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 117, in migrate state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 147, in _migrate_all_forwards state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/executor.py", line 245, in apply_migration state = migration.apply(state, schema_editor) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/migration.py", line 124, in apply operation.database_forwards(self.app_label, schema_editor, old_state, project_state) File "/usr/local/lib/python3.6/dist-packages/django/db/migrations/operations/fields.py", line 249, in database_forwards schema_editor.alter_field(from_model, from_field, to_field) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 565, in alter_field old_db_params, new_db_params, strict) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 745, in _alter_field self.execute(self._create_unique_sql(model, [new_field.column])) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/base/schema.py", line 142, in execute cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 100, in execute return super().execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 68, in execute return self._execute_with_wrappers(sql, params, many=False, executor=self._execute) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 77, in _execute_with_wrappers return executor(sql, params, many, context) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/utils.py", line 90, in __exit__ raise dj_exc_value.with_traceback(traceback) from exc_value File "/usr/local/lib/python3.6/dist-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) File "/usr/local/lib/python3.6/dist-packages/django/db/backends/mysql/base.py", line 74, in execute return self.cursor.execute(query, args) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 209, in execute res = self._query(query) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/cursors.py", line 315, in _query db.query(q) File "/usr/local/lib/python3.6/dist-packages/MySQLdb/connections.py", line 239, in query _mysql.connection.query(self, query) django.db.utils.OperationalError: (1061, "Duplicate key name 'testapp_mymodel_name_ba5e2bd2_uniq'") wangx@aliyun:~/testunique$ python3 manage.py sqlmigrate testapp 0002 -- -- Alter field name on mymodel -- ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq` UNIQUE (`name`); -- -- Alter unique_together for mymodel (0 constraint(s)) -- ALTER TABLE `testapp_mymodel` DROP INDEX `testapp_mymodel_name_ba5e2bd2_uniq`; wangx@aliyun:~/testunique$ mysql -u test -p Enter password: Welcome to the MySQL monitor. Commands end with ; or \g. Your MySQL connection id is 36 Server version: 5.7.29-0ubuntu0.18.04.1 (Ubuntu) Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved. Oracle is a registered trademark of Oracle Corporation and/or its affiliates. Other names may be trademarks of their respective owners. Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. mysql> use testunique; Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Database changed mysql> ALTER TABLE `testapp_mymodel` DROP INDEX `testapp_mymodel_name_ba5e2bd2_uniq`; Query OK, 0 rows affected (0.01 sec) Records: 0 Duplicates: 0 Warnings: 0 mysql> ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq` UNIQUE (`name`); Query OK, 0 rows affected (0.01 sec) Records: 0 Duplicates: 0 Warnings: 0 mysql> ALTER TABLE `testapp_mymodel` ADD CONSTRAINT `testapp_mymodel_name_ba5e2bd2_uniq_by_unique_together` UNIQUE (`name`); Query OK, 0 rows affected, 1 warning (0.00 sec) Records: 0 Duplicates: 0 Warnings: 1
comment:3 by , 5 years ago
I think we can consider limit the length of unique_together. If a user set unique_together containing only one field, we can raise an error and ask him to use unique=True instead.
comment:4 by , 3 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Hi!
I looked a bit into the issue. I'll share my thoughts and suggest/discuss some solutions so that we can agree on the approach. I'd be more than happy to tackle the issue then, if that is okay, once we know how we want to fix the issue :D
TL;DR: some suboptimal solutions are presented, but 5/ and 6/ look the most promising to me. Please let's discuss those a bit at least :)
So the issue occurs because we are changing an unique_together
to a unique=True
on the field (similarly, I believe the bug occurs with index_together
and db_index
), which will first try to create an existing index before deleting it.
Some solutions:
1/ Changing the index name
I think changing the format of index name if the CONSTRAINT is create by the unique_together will work either.
It would somehow work and mitigate the issue at hand, but it might introduce complexity for backward compatibility. When upgrading your Django version and wanting to drop an index, Django will have to know whether the name of the index comes the previous or current version of Django, in order to know how the index is called and drop it.
2/ Forbid using unique_together
(and index_together
) with a single field
If a user set unique_together containing only one field, we can raise an error and ask him to use unique=True instead
It feels more like a workaround than a real fix of the issue. And more generally, we will have issues with backward compatibility. We can't break unique_together
s with one field from a release to the next. I guess we would need to add a deprecation warning, and really introduce a breaking behaviour in the next major release (Django 5.x then?). Which seems IMHO like a lot of trouble for a rather narrow issue.
3/ Leverage the existing foo_together_change
dependency mecanism
The autodetector has a similar re-order behaviour so the one we would need already implemented. When dropping a field, we add a dependency called foo_together_change
to the field, which would re-order the AlterUniqueTogether
operations, for the index to be dropped before the removal of the field.
I tried it out for field altering (see code block below), and it would fix the issue at hand, but it wouldn't fix the reverse operation. If we changed from a unique=True
to a unique_together
, the dependency would still re-order the AlterUniqueTogether
operation to happen before the AlterField
, but we would need to first drop the index through the AlterField
.
So the very same issue occurs, just the other way around.
diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 2848adce7d..598d4649e9 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -987,7 +987,9 @@ class MigrationAutodetector: field=field, preserve_default=preserve_default, ), - dependencies=dependencies, + dependencies=dependencies + [ + (app_label, model_name, field_name, "foo_together_change"), + ], ) else: # We cannot alter between m2m and concrete fields
4/ Split the index dropping/creation out of the AlterField operation
The bug seems related to the fact that AlterField
does a lot of things, and among them is the creation/drop of indexes, which can clash with other structures.
So one could probably move this part out of AlterField
, but it feels like a very heavy and error-prone change to a part that is currently core to the autodetector and migrations framework.
This idea is a long-shot, and also pretty vague in my head. I wouldn't actually consider this solution.
5/ Do multi-step AlterFooTogether operations
This solution, and the next one, focus on the idea that index creation should operate in two steps (in migrations). First, index drops, and then, after field removal/creation/altering, add indexes.
Today, AlterUniqueTogether
is one step that will both create and drop indexes, which is a part of the problem. So would de-couple this, and first do the AlterFooTogether
that would drop indexes, then field changes, and then again AlterFooTogether
to add indexes.
A small issue is that this operation is declarative with respect to the expected model state, we just go from one state to the next.
So the idea would be to split the AlterFooTogether
operation (in two mostly), so that the migration ends up in the correct state, but with an intermediate state that, by design, only drops indexes.
An example would be (in pseudo-code):
Initial model
class MyModel(models.Model): name = models.CharField(max_length=32) age = models.IntegerField() class Meta: unique_together = ("name", )
becomes:
class MyModel(models.Model): name = models.CharField(max_length=32, unique=True) age = models.IntegerField() class Meta: unique_together = ("age", )
would do operations like
operations = [ # Dropping the "name" index migrations.AlterUniqueTogether( name='mymodel', unique_together=set(), ), # Adding the "name" index migrations.AlterField( model_name='mymodel', name='name', field=models.CharField(max_length=32, unique=True), ), # Adding the "age" index migrations.AlterUniqueTogether( name='mymodel', unique_together={("age", )}, ), ]
(one could also imagine age
being a unique=True
first, where the AlterField
will drop the index)
I believe the amount of work is not that high for this solution, and we should have no issue with backward compatibility, since we keep the same logic for the operation itself.
The tricky part is to the generate the intermediate state of foo_together, that will only drop the related indexes.
An issue I see: we use implicitly the AlterFooTogether
behaviour to serve our purpose, and not the purpose it was made for.
6/ Introduce explicit CreateFooTogether and DropFooTogether operations
The cleaner solution to 5/ would be to introduce four new operations, creating and dropping unique_together and index_together. This would mimic what is done for constraints and indexes in the autodetector, having two separate steps.
The issue is that, a) it will introduce a lot of new code and operations. Even if the code is not complicated, it's still content to document and maintain. And b) for backward compatibility, we would need to keep AlterFooTogether
operations (forever?) the way they are, even if the operation is not generated by Django anymore.
End note:
From a quick look, 5/ seems like the most realistic approach, but I'd like to confirm with more experienced Django contributors/maintainers :)
Thanks!
David
comment:5 by , 3 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
A first PR https://github.com/django/django/pull/14722 with approach 5/ (from my comment above) basically.
comment:6 by , 3 years ago
Patch needs improvement: | unset |
---|
Removing "patch needs improvement" to get some eyes to look at it :)
comment:7 by , 3 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:8 by , 3 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:9 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
this is a small project which can repeat the bug.