Opened 10 years ago
Closed 5 weeks ago
#26149 closed Bug (fixed)
Invalid migration generated when using order_with_respect_to and a unique_together constraint
| Reported by: | Richard Xia | Owned by: | |
|---|---|---|---|
| Component: | Migrations | Version: | 1.9 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
I ran into a bug when trying to generate and apply a migration for a model that has the order_with_respect_to option set along with a unique_together which involves the implicit _order field created by order_with_respect_to. For example, if you have the following models:
class Bar(models.Model): pass class Foo(models.Model): class Meta: order_with_respect_to = 'bar' unique_together = ('bar', '_order') bar = models.ForeignKey(Bar)
when you generate the migrations with make_migrations, it creates a migration with the operations out of order. It attempts to run the AlterUniqueTogether operation before the AlterOrderWithRespectTo, which is problematic because the implicit _order field is created by the latter operation but the former operation tries to create a uniqueness constraint using it. This results in the error: django.core.exceptions.FieldDoesNotExist: Foo has no field named u'_order'
For convenience, I've created a Github repo that sets up the models, so that to reproduce the bug, all you have to do is run makemigrations followed by migrate: https://github.com/richardxia/django-migration-bug
Attachments (2)
Change History (13)
comment:1 by , 10 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
by , 10 years ago
| Attachment: | migration-bug.patch added |
|---|
comment:2 by , 10 years ago
Hello,
Could you check this patch, please?
I changed "db/migrations/autodetector.py" and now there is no bug.
python manage.py makemigrations
Migrations for 'p1':
p1/migrations/0001_initial.py:
Create model Bar
Create model Foo
Set order_with_respect_to on foo to bar
Alter unique_together for foo (1 constraint(s))
python manage.py migrate
Operations to perform:
Apply all migrations: admin, auth, contenttypes, p1, sessions
Running migrations:
Rendering model states... DONE
Applying contenttypes.0001_initial... OK
Applying auth.0001_initial... OK
Applying admin.0001_initial... OK
Applying admin.0002_logentry_remove_auto_add... 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 p1.0001_initial... OK
Applying sessions.0001_initial... OK
Thanks
comment:3 by , 10 years ago
| Has patch: | set |
|---|---|
| Needs tests: | set |
A test will be needed, probably in tests/migrations/test_autodetector.py. If the patch fixes the test and doesn't break any other tests, it might be a good candidate!
by , 10 years ago
| Attachment: | migration-bug-twotest.patch added |
|---|
follow-up: 6 comment:5 by , 10 years ago
Hello,
I did a new attachment with the test and patch, could you check, please? Could I do the PR?
Thank you.
comment:6 by , 10 years ago
Replying to anabelensc:
Hello,
I did a new attachment with the test and patch, could you check, please? Could I do the PR?
Thank you.
Hi, I think there might be a better way to fix this bug. When computing dependencies for index_together and unique_together fields, the autodetector seems to add dependencies on all the related fields on the model, rather than dependencies on the actual fields in the compound index, which might not be related fields at all. They should depend on exactly the fields they index and check_dependencies should return True if the operation is AlterOrderWithRespectTo and the dependency is on the _order fields.
comment:7 by , 10 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:8 by , 9 years ago
| Needs tests: | unset |
|---|---|
| Patch needs improvement: | set |
PR with the attached patch, but there's no discussion about why that solution is preferred despite the previous comment.
comment:9 by , 3 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:10 by , 5 weeks ago
I guess this bug don't exist no more. As I checked your sample code and migration done without Exception.
your code sample: just on_delete added to dependancy.
class Bar(models.Model):
pass
class Foo(models.Model):
class Meta:
order_with_respect_to = 'bar'
unique_together = ('bar', '_order')
bar = models.ForeignKey(Bar, on_delete=models.CASCADE)
below is the migration generated:
class Migration(migrations.Migration):
initial = True
dependencies = []
operations = [
migrations.CreateModel(
name="Bar",
fields=[
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
],
),
migrations.CreateModel(
name="Foo",
fields=[
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
(
"bar",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to="myapp.bar"
),
),
],
options={
"order_with_respect_to": "bar",
"unique_together": {("bar", "_order")},
},
),
]
comment:11 by , 5 weeks ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Thanks for the sample project!