#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)
Change History (24)
by , 20 months ago
Attachment: | patch.diff added |
---|
comment:1 by , 20 months ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:2 by , 20 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 , 20 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 , 20 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.
follow-up: 7 comment:5 by , 20 months ago
Triage Stage: | Unreviewed → Accepted |
---|
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 , 20 months ago
Cc: | added |
---|
comment:7 by , 20 months ago
Severity: | Normal → Release 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 , 20 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 , 20 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.
follow-up: 11 comment:10 by , 20 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.
comment:11 by , 20 months ago
Replying to Natalia Bidart:
Well of course the warning is not present in
4.1
since the deprecation was added in4.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 , 20 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 , 20 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 , 20 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 , 20 months ago
Cc: | 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.
comment:16 by , 20 months ago
Cc: | added |
---|
follow-up: 18 comment:17 by , 20 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?
comment:18 by , 20 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:22 by , 19 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:
- Follow the guidance in the release notes to get a new migration with
RenameIndex
operation. - Apply migrations.
- Add
new_name
fromRenameIndex
operations to theMeta.indexes
, e.g. when you havemigrations.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 theIndex
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. - Remove
options["index_together"]
from theCreateModel()
operation and addoptions["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, changeAlterIndexTogether()
toAddIndex()
. - Remove
AlterIndexTogether()
andRenameIndex()
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.
comment:23 by , 19 months ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
Resolution: | → invalid |
Status: | new → closed |
Triage Stage: | Accepted → Unreviewed |
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
.
suggestetd patch (without tests)