Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#34525 closed Bug (invalid)

index_together warning after migration to new style

Reported by: Mateusz Legięcki Owned by: nobody
Component: Database layer (models, ORM) Version: 4.2
Severity: Release blocker Keywords: index_together, warning
Cc: Natalia Bidart, Simon Charette, Francesco Panico Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

patch.diff (631 bytes ) - added by Mateusz Legięcki 11 months ago.
suggestetd patch (without tests)

Download all attachments as: .zip

Change History (24)

by Mateusz Legięcki, 11 months ago

Attachment: patch.diff added

suggestetd patch (without tests)

comment:1 by Mateusz Legięcki, 11 months ago

Has patch: set
Patch needs improvement: set

comment:2 by Natalia Bidart, 11 months ago

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/

comment:3 by Natalia Bidart, 11 months ago

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.

comment:4 by Tim Graham, 11 months ago

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.

comment:5 by Natalia Bidart, 11 months ago

Triage Stage: UnreviewedAccepted

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

comment:6 by Natalia Bidart, 11 months ago

Cc: Natalia Bidart added

in reply to:  5 comment:7 by Mariusz Felisiak, 11 months ago

Severity: NormalRelease blocker

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

comment:8 by Mariusz Felisiak, 11 months ago

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?

comment:9 by Natalia Bidart, 11 months ago

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.

comment:10 by Natalia Bidart, 11 months ago

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.

in reply to:  10 comment:11 by Mariusz Felisiak, 11 months ago

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

comment:12 by Natalia Bidart, 11 months ago

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

comment:13 by Tim Graham, 11 months ago

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

comment:14 by Natalia Bidart, 11 months ago

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.

comment:15 by Mariusz Felisiak, 11 months ago

Cc: Simon Charette added

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.

Last edited 11 months ago by Mariusz Felisiak (previous) (diff)

comment:16 by Francesco Panico, 11 months ago

Cc: Francesco Panico added

comment:17 by Simon Charette, 11 months ago

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?

in reply to:  17 comment:18 by Mariusz Felisiak, 11 months ago

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.

comment:19 by Mariusz Felisiak, 11 months ago

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

comment:20 by GitHub <noreply@…>, 11 months ago

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.

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

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.

comment:22 by Mariusz Felisiak, 11 months ago

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"),
                    ],
                },
            ),
        ]
    
    if field used in an index was added later, change AlterIndexTogether() to AddIndex().
  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.

Last edited 11 months ago by Mariusz Felisiak (previous) (diff)

comment:23 by Mariusz Felisiak, 11 months ago

Has patch: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed
Triage Stage: AcceptedUnreviewed

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.

Note: See TracTickets for help on using tickets.
Back to Top