index_together warning after migration to new style

[Mateusz Legięcki]

I've got deprecation warning about index_together after migration to new style with model.Index(fields=[...]). I'm using pytest as test suite. I created minimal example, it is available on my GH: ​https://github.com/Behoston/idx_to_bug

logs:

x/tests.py::test_x
  /home/behoston/venv/idx_to/lib/python3.11/site-packages/django/db/models/options.py:210: RemovedInDjango51Warning: 'index_together' is deprecated. Use 'Meta.indexes' in 'x.Item' instead.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

For fresh indexes it works fine (no warning), so I assume that problem is only during migrations when models from migrations files are evaluated.

[Mateusz Legięcki]

suggestetd patch (without tests)

[Natalia Bidart]

Hello! Thank you for your contribution.

Would you have the time to propose the patch as GitHub PR? Here is the documentation explaining how to submit contributions: ​https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/

[Natalia Bidart]

Actually let me try to reproduce first to confirm whether this ticket should be Accepted or not, so you don't need to prepare the PR is there is something else going on.

[Tim Graham]

Based on ​the deprecation note in the 4.2 release notes, it's unclear whether or not the plan is to support index_together in historical migrations.

[Natalia Bidart]

I'm able to reproduce the warning using latest main and the Django test framework. But removing the migrations from the test repo and starting fresh does not raise the warning (as expected).

Thank you Tim for the pointer to the release notes, while it's true nothing is said about supporting index_together in older migrations, IMHO it would be ideal if the RemovedInDjango51Warning is not emitted for code bases that were already migrated out of the deprecated feature.

I'm not familiar with migrations but I think this is a valid report, accepting (at least until with more experience in this topic decides otherwise).

[Mariusz Felisiak]

Replying to Natalia Bidart:

I'm not familiar with migrations but I think this is a valid report, accepting (at least until with more experience in this topic decides otherwise).

If it is a regression in Django 4.2 it should be marked as a release blocker (I didn't check it.)

[Mariusz Felisiak]

Our plan (as usually when we deprecate/remove features from the ORM) was to support Meta.index_together only in historical migrations (already applied), see ​DEFAULT_NAMES.

Is the warning triggered for already applied migrations?

[Natalia Bidart]

Mariusz: the warning is shown every time the tests are run, and the index_together is declared in the 0001_initial.py migration. This may be caused by the test database setup, when migrations are run, so perhaps the answer to you question is "no" because the initial migration is being applied in the test database. When running the dev server or other operations (makemigrations, migrate, etc.), the warning is not printed. This is why I did not consider it a release blocker, but a "nice to have" (IMHO).

This happens for 4.2 and main, will check against 4.1 and report back.

[Natalia Bidart]

Well of course the warning is not present in 4.1 since the deprecation was added in 4.2 :-D. I still don't think this is a release blocker though.

[Mariusz Felisiak]

Replying to Natalia Bidart:

Well of course the warning is not present in 4.1 since the deprecation was added in 4.2 :-D. I still don't think this is a release blocker though.

If you consider it a bug then it is a release blocker, it doesn't matter if it's "big" or "small".

[Natalia Bidart]

I'm getting familiar with ticket:27236#comment:31, which seems extremely relevant for making further decisions on this one.

[Tim Graham]

Avoiding spurious deprecation warnings in historical migrations is one reason that model fields are deprecated using the system check framework. I wonder if this approach was considered for this deprecation (although the proposed patch might work too).

[Natalia Bidart]

I would like to add Simon as CC so we can hear their opinion, but it seems I can't add people as CC (other than myself). I will ask Mariusz for the extra perms next week.

[Mariusz Felisiak]

To be clear, I believe that we wanted to do the same for AlterIndexTogether() and CreateModel()'s index_together i.e. support them only for pre-Django 4.2 migration files and don't deprecate or remove them. That's why we should avoid raising deprecation warnings for model states and add a similar ​warning to the CreateModel() docs.

[Simon Charette]

I think the situation here is slightly different than with CommaSeparatedIntegerField where we ​used system checks to allow historical migrations to keep referencing such fields.

With the planed complete removal of index_together support in Django 5.1 I believe that using a system check instead of deprecation warning would be doing a disservice to users as that would convey that a shim is going to remain in place for releases to come (current situation with CommaSeparatedIntegerField). Users will have to squash or edit their migration to support the next release of Django, it is inevitable.

I think that the case of ForeignKey.on_delete promotion to a required kwarg in is a better analogous to the situation we are facing here (#21127) which required manual adjustments of historic migrations #28677 ​which didn't seem contentious at the time?

[Mariusz Felisiak]

Replying to Simon Charette:

Users will have to squash or edit their migration to support the next release of Django, it is inevitable.

We should implement set of reductions to make it doable for users. I've prepared a ​branch (no tests and release notes but I'll be happy to add them) that allows me to optimized a squashed migration (0001-0004) in a ​quite complicated situation from:

    operations = [
        migrations.CreateModel(
            name='MyModel',
            fields=[
                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('field_1', models.IntegerField()),
                ('field_2', models.TextField()),
                ('field_3', models.IntegerField()),
                ('field_4', models.IntegerField(null=True)),
            ],
            options={
                'index_together': {('field_2', 'field_4')},
            },
        ),
        migrations.AddIndex(
            model_name='mymodel',
            index=models.Index(fields=['field_1', 'field_2'], name='test_one_my_field_1_ea7372_idx'),
        ),
        migrations.AddIndex(
            model_name='mymodel',
            index=models.Index(fields=['field_3'], name='test_one_my_field_3_d91b6c_idx'),
        ),
        migrations.AddIndex(
            model_name='mymodel',
            index=models.Index(fields=['field_3', 'field_4'], name='test_one_my_field_3_1e8bd9_idx'),
        ),
        migrations.RemoveIndex(
            model_name='mymodel',
            name='test_one_my_field_1_ea7372_idx',
        ),
        migrations.AlterIndexTogether(
            name='mymodel',
            index_together={('field_1', 'field_2'), ('field_2', 'field_4')},
        ),
        migrations.RenameIndex(
            model_name='mymodel',
            new_name='test_one_my_field_1_ea7372_idx',
            old_fields=('field_1', 'field_2'),
        ),
        migrations.RenameIndex(
            model_name='mymodel',
            new_name='test_one_my_field_2_26d2ae_idx',
            old_fields=('field_2', 'field_4'),
        ),
    ]

into

    operations = [
        migrations.CreateModel(
            name='MyModel',
            fields=[
                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('field_1', models.IntegerField()),
                ('field_2', models.TextField()),
                ('field_3', models.IntegerField()),
                ('field_4', models.IntegerField(null=True)),
            ],
            options={
                'indexes': [
                    models.Index(fields=['field_3'], name='test_one_my_field_3_d91b6c_idx'),
                    models.Index(fields=['field_3', 'field_4'], name='test_one_my_field_3_1e8bd9_idx'),
                    models.Index(fields=['field_1', 'field_2'], name='test_one_my_field_1_ea7372_idx'),
                    models.Index(fields=['field_2', 'field_4'], name='test_one_my_field_2_26d2ae_idx')
                ],
            },
        ),
    ]

without index_together 🎉 As far as I'm aware, once this is implemented we can start recommending squashing to end users.

[Mariusz Felisiak]

I think we can close it as invalid when #34529 is resolved.

[GitHub <noreply@…>]

In 8e2460d5:

Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations.

This makes squashing migrations an available path for changing
Meta.index_together, which is deprecated, to Meta.indexes.

Follow up to f81032572107846922745b68d5b7191058fdd5f5.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 290fd5ec:

[4.2.x] Fixed #34529, Refs #34525 -- Reduced index operations with Meta.indexes/index_together when optimizing migrations.

This makes squashing migrations an available path for changing
Meta.index_together, which is deprecated, to Meta.indexes.

Follow up to f81032572107846922745b68d5b7191058fdd5f5.

Backport of 8e2460d599aec95f8cfe514d3cc8acdd4ca4b1fb from main.

[Mariusz Felisiak]

If squashing migrations is not an option for you, then you have to manually adjust historical migrations. I'd do this in the following steps:

1. Follow the guidance in the ​release notes to get a new migration with RenameIndex operation.
2. Apply migrations.
3. Add new_name from RenameIndex operations to the Meta.indexes, e.g. when you have

   migrations.RenameIndex(
       model_name="mymodel",
       new_name="test_one_my_field_1_ea7372_idx",
       old_fields=("field_1", "field_2"),
   ),
   

   add "test_one_my_field_1_ea7372_idx" to the Index definition:

   class MyModel(models.Model):
       ...
       class Meta:
           indexes = [
               models.Index(fields=['field_1', 'field_2'], name="test_one_my_field_1_ea7372_idx"),
           ]
   

   makemigrations should not detect any changes.
4. Remove options["index_together"] from the CreateModel() operation and add options["indexes"] instead, e.g.

       operations = [
           migrations.CreateModel(
               name='MyModel',
               fields=[
                   ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                   ('field_1', models.IntegerField()),
                   ('field_2', models.TextField()),
                   ('field_3', models.IntegerField()),
               ],
               options={
                   "indexes": [
                       models.Index(fields=['field_1', 'field_2'], name="test_one_my_field_1_ea7372_idx"),
                   ],
               },
           ),
       ]
   

5. Remove AlterIndexTogether() and RenameIndex() operations related with this index. makemigrations should not detect any changes.

Some minor adjustments may be needed but it's the general guidelines that I will follow.

[Mariusz Felisiak]

Deprecation warnings are not spurious as we will not support index_together in migrations when the deprecation ends. You can squash migrations (see #34529) or adjust them manually to get rid of index_together.