[Django] #26149: Invalid migration generated when using order_with_respect_to and a unique_together constraint
Django
unique_together constraint
----------------------------+--------------------
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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:
{{{#!python
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26149>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Thanks for the sample project!
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:1>
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anabelensc):
* Attachment "migration-bug.patch" added.
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anabelensc):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:2>
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:3>
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anabelensc):
thank you, I am working on it .
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:4>
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anabelensc):
* Attachment "migration-bug-twotest.patch" added.
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anabelensc):
Hello,
I did a new attachment with the test and patch, could you check, please?
Could I do the PR?
Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:5>
Django
unique_together constraint
Reporter: richardxia | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jdavisp3):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:6>
Django
unique_together constraint
Reporter: richardxia | Owner: anabelensc
Type: Bug | Status: assigned
Component: Migrations | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anabelensc):
* owner: nobody => anabelensc
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:7>
Django
unique_together constraint
Reporter: richardxia | Owner: anabelensc
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
[https://github.com/django/django/pull/7053 PR] with the attached patch,
but there's no discussion about why that solution is preferred despite the
previous comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:8>
Django
unique_together constraint
Reporter: Richard Xia | Owner: (none)
Type: Bug | Status: new
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Mariusz Felisiak):
* owner: anabelensc => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:9>
Django
unique_together constraint
Reporter: Richard Xia | Owner: (none)
Component: Migrations | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by amir jamshidi):
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')
}}}
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")},
},
),
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:10>
Django
unique_together constraint
Reporter: Richard Xia | Owner: (none)
Component: Migrations | Version: 1.9
Severity: Normal | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by amir jamshidi):
* resolution: => fixed
* status: new => closed
--
Ticket URL: <https://code.djangoproject.com/ticket/26149#comment:11>