#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 , 6 years ago
| Attachment: | testunique.tar.gz added |
|---|
comment:1 by , 6 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 , 6 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 , 6 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 , 4 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_togethers 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 , 4 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 , 4 years ago
| Patch needs improvement: | unset |
|---|
Removing "patch needs improvement" to get some eyes to look at it :)
comment:7 by , 4 years ago
| Needs tests: | set |
|---|---|
| Patch needs improvement: | set |
comment:8 by , 4 years ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | unset |
comment:9 by , 4 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
this is a small project which can repeat the bug.