Models migration with change field foreign to many and deleting unique together.

[budzichd]

I have models like

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

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.

I added attachment with this project, download it and then do makemigrations and then migrate to see this error.

[budzichd]

Download this file and then do makemigrations and migrate to see this error.

[Mariusz Felisiak]

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.

[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

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

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

# 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

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',
    ),
]

[David Wobrock]

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 ​PR.

[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.

[GitHub <noreply@…>]

In 798b6c2:

Fixed #31788 -- Fixed migration optimization after altering field to ManyToManyField.

This makes AddField() used for altering to ManyToManyField, dependent
on the prior RemoveField.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 9fce76a2:

[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