[Django] #31788: Models migration with change field foreign to many and deleting unique together.
Django
together.
--------------------------------------+------------------------
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
I have models like
{{{
class Authors(models.Model):
project_data_set = models.ManyToManyField(
ProjectDataSet,
on_delete=models.PROTECT
)
state = models.IntegerField()
start_date = models.DateField()
class Meta:
unique_together = (('project_data_set', 'state', 'start_date'),)
}}}
and
{{{
class DataSet(models.Model):
name = models.TextField(max_length=50)
class Project(models.Model):
data_sets = models.ManyToManyField(
DataSet,
through='ProjectDataSet',
)
name = models.TextField(max_length=50)
class ProjectDataSet(models.Model):
"""
Cross table of data set and project
"""
data_set = models.ForeignKey(DataSet, on_delete=models.PROTECT)
project = models.ForeignKey(Project, on_delete=models.PROTECT)
class Meta:
unique_together = (('data_set', 'project'),)
}}}
when i want to change field project_data_set in Authors model from foreign
key field to many to many field I must delete a unique_together, cause it
can't be on many to many field.
Then my model should be like:
{{{
class Authors(models.Model):
project_data_set = models.ManyToManyField(
ProjectDataSet,
)
state = models.IntegerField()
start_date = models.DateField()
}}}
But when I want to do a migrations.
1. python3 manage.py makemigrations
2. python3 manage.py migrate
I have error:
{{{ValueError: Found wrong number (0) of constraints for
app_authors(project_data_set, state, start_date)}}}
The database is on production, so I can't delete previous initial
migrations, and this error isn't depending on database, cause I delete it
and error is still the same.
My solve is to first delete unique_together, then do a makemigrations and
then migrate. After that change the field from foreign key to many to many
field, then do a makemigrations and then migrate.
But in this way I have 2 migrations instead of one.
--
Ticket URL: <https://code.djangoproject.com/ticket/31788>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
together.
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by budzichd):
* Attachment "ticket-django.zip" added.
Download this file and then do makemigrations and migrate to see this
error.
Django
together.
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by budzichd:
Old description:
New description:
I have models like
{{{
class Authors(models.Model):
and
{{{
class Authors(models.Model):
I add attachment with this project, download it and then do makemigrations
and then migrate to see this error.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:1>
Django
together.
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by budzichd:
Old description:
> I have models like
> I add attachment with this project, download it and then do
> makemigrations and then migrate to see this error.
New description:
I have models like
{{{
class Authors(models.Model):
and
{{{
class Authors(models.Model):
I added attachment with this project, download it and then do
makemigrations and then migrate to see this error.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:2>
Django
together.
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
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 felixxm):
* cc: Markus Holtermann, Simon Charette (added)
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. Tentatively accepting, however I'm not sure if we
can sort these operations properly, we should probably alter
`unique_together` first
{{{
migrations.AlterUniqueTogether(
name='authors',
unique_together=set(),
),
migrations.RemoveField(
model_name='authors',
name='project_data_set',
),
migrations.AddField(
model_name='authors',
name='project_data_set',
field=models.ManyToManyField(to='dict.ProjectDataSet'),
),
}}}
You should take into account that you'll lose all data because
``ForeignKey`` cannot be altered to ``ManyToManyField``.
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:3>
Django
together.
Reporter: budzichd | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
> I added attachment with this project, download it and then do
> makemigrations and then migrate to see this error.
New description:
I have models like
{{{#!python
class Authors(models.Model):
project_data_set = models.ForeignKey(
and
{{{
class Authors(models.Model):
I added attachment with this project, download it and then do
makemigrations and then migrate to see this error.
--
Comment (by Simon Charette):
I agree that you'll loose data but `Alter(Index|Unique)Together` should
always be sorted before `RemoveField`
https://github.com/django/django/blob/b502061027b90499f2e20210f944292cecd74d24/django/db/migrations/autodetector.py#L910
https://github.com/django/django/blob/b502061027b90499f2e20210f944292cecd74d24/django/db/migrations/autodetector.py#L424-L430
So something's broken here in a few different ways and I suspect it's due
to the fact the same field name `project_data_set` is reused for the many-
to-many field.
If you start from
{{{#!python
class Authors(models.Model):
project_data_set = models.ForeignKey(
ProjectDataSet,
on_delete=models.PROTECT
)
state = models.IntegerField()
start_date = models.DateField()
class Meta:
unique_together = (('project_data_set', 'state', 'start_date'),)
}}}
And generate `makemigrations` for
{{{#!python
class Authors(models.Model):
project_data_set = models.ManyToManyField(ProjectDataSet)
state = models.IntegerField()
start_date = models.DateField()
}}}
You'll get two migrations with the following operations
{{{#!python
# 0002
operations = [
migrations.AddField(
model_name='authors',
name='project_data_set',
field=models.ManyToManyField(to='ticket_31788.ProjectDataSet'),
),
migrations.AlterUniqueTogether(
name='authors',
unique_together=set(),
),
migrations.RemoveField(
model_name='authors',
name='project_data_set',
),
]
# 0003
operations = [
migrations.AddField(
model_name='authors',
name='project_data_set',
field=models.ManyToManyField(to='ticket_31788.ProjectDataSet'),
),
]
}}}
If you change the name of the field to something else like
`project_data_sets` every work as expected
{{{#!python
operations = [
migrations.AddField(
model_name='authors',
name='project_data_sets',
field=models.ManyToManyField(to='ticket_31788.ProjectDataSet'),
),
migrations.AlterUniqueTogether(
name='authors',
unique_together=set(),
),
migrations.RemoveField(
model_name='authors',
name='project_data_set',
),
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:4>
Django
together.
Reporter: budzichd | Owner: David Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* has_patch: 0 => 1
* status: new => assigned
Comment:
Quite a funny bug actually in the autodetector and how we sort migration
with respect to their dependencies.
I can't exactly reproduce the described output, but something similar.
Here is a little dive into the issue.
When changing a field from FK to M2M, the autodetector goes within
`django.db.migrations.autodetector.MigrationAutodetector.generate_altered_fields`
and creates a `RemoveField`+`AddField`, which is fine and works.
But we also generate a `AlterUniqueTogether`...
Since we call `generate_removed_altered_unique_together` before
`generate_altered_fields`, one would expect it to work as expected.
And at first, we have the right operation order: `[AlterUniqueTogether,
RemoveField, AddField]`.
However, when sorting the operations with respect to their dependencies,
we trigger the dependency that `RemoveField` should be after
`AlterUniqueTogether`.
Even if this condition is already satisfied, the `stable_topological_sort`
changes the order and returns `[AlterUniqueTogether, AddField,
RemoveField]`. The dependency is still satisfied, but our Remove/Add
became an Add/Remove, which cannot work.
In `tests/utils_tests/test_topological_sort.py`
{{{
def test_preserve_satisfied_order(self):
dependency_graph = {1: set(), 2: {1}, 3: set()}
self.assertEqual(
stable_topological_sort([1, 2, 3], dependency_graph),
[1, 2, 3],
)
}}}
return
{{{
======================================================================
FAIL: test_preserve_satisfied_order
(utils_tests.test_topological_sort.TopologicalSortTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/david/django/tests/utils_tests/test_topological_sort.py",
line 29, in test_preserve_satisfied_order
self.assertEqual(
AssertionError: Lists differ: [1, 3, 2] != [1, 2, 3]
First differing element 1:
3
2
- [1, 3, 2]
+ [1, 2, 3]
}}}
on my env (and since we rely on sets, I suspect that's why I don't have
the same result as described in the ticket. I think that's why Simon is
seeing a `[AddField, AlterUniqueTogether, RemoveField]`, which also
satisfies the dependencies).
(on top of that, the autodetector then optimizes the operations, and the
`AddField` followed by `RemoveField` cancel each other out, and we end up
with a single `[AlterUniqueTogether]`).
Two possible approaches to fix this:
1. Making the `AddField` depend on `RemoveField`, so that we force their
order
2. Changing the topological sort, so that it preserves the input order as
much as possible
Since the second might imply a large set of changes, so I tried
implementing the first one in this
[https://github.com/django/django/pull/15738 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:5>
Django
together.
Reporter: budzichd | Owner: David Wobrock
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Thanks for the investigation!
> 2. Changing the topological sort, so that it preserves the input order
as much as possible
I think it's not doable. That's just how the topological sort works. I've
checked with `graphlib.TopologicalSorter()` (Python 3.9+) and it works
exactly the same.
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:6>
Django
together.
Reporter: budzichd | Owner: David Wobrock
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:7>
Django
together.
Reporter: budzichd | Owner: David Wobrock
Component: Migrations | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"798b6c23ee52c675dd0f0e233c50cddd7ff15657" 798b6c2]:
{{{
#!CommitTicketReference repository=""
revision="798b6c23ee52c675dd0f0e233c50cddd7ff15657"
Fixed #31788 -- Fixed migration optimization after altering field to
ManyToManyField.
This makes AddField() used for altering to ManyToManyField, dependent
on the prior RemoveField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:8>
Django
together.
Reporter: budzichd | Owner: David Wobrock
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9fce76a237ddf19e167dcba53815cbfee586b928" 9fce76a2]:
{{{
#!CommitTicketReference repository=""
revision="9fce76a237ddf19e167dcba53815cbfee586b928"
[4.1.x] Fixed #31788 -- Fixed migration optimization after altering field
to ManyToManyField.
This makes AddField() used for altering to ManyToManyField, dependent
on the prior RemoveField.
Backport of 798b6c23ee52c675dd0f0e233c50cddd7ff15657 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31788#comment:9>