#34856 closed Bug (invalid)
Running tests with historical migrations that contain index together fails with TypeError.
Reported by: | Sage Abdullah | Owned by: | nobody |
---|---|---|---|
Component: | Migrations | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When running tests against Django's main
branch on a project that has a historical migration that contains index_together
(either via the AlterIndexTogether
operation or as part of a CreateModel
operation), running that migration fails with TypeError: 'class Meta' got invalid attribute(s): index_together
. This is the case even if the model has already been updated to use indexes
instead of index_together
and new migrations have been created to update it.
To reproduce:
- Install Django < 5.1
- Start a new project and a new app
- Create a model with
index_together
and runmakemigrations
- or run
makemigrations
before addingindex_together
and run it again soAlterIndexTogether
operation is created, same result
- or run
- Write a test that uses the database (a simple test with
TestCase
and notSimpleTestCase
should suffice) - Optional: Update the model to use
indexes
instead ofindex_together
and runmakemigrations
- Install latest Django
main
- Run
python manage.py test -v3
and it fails when applying the migration that containsindex_together
Reproducible example: https://github.com/laymonage/django-test-repro
This is likely because "index_together"
has been removed from django.db.models.options.DEFAULT_NAMES
in 2abf417c815c20f41c0868d6f66520b32347106e. Meanwhile, the historical model constructed by the migrations framework will try to build the model with the Meta.index_together
option.
I'm not sure if this is expected. If it is, then that means developers have to update their old migrations to not use index_together
at all, which doesn't seem to be ideal.
If it is not, the solution might be to reintroduce "index_together"
in DEFAULT_NAMES
even though it will be no-op. Not sure if that's all there is to it, but I'm happy to make a PR for a start.
Change History (5)
comment:1 by , 14 months ago
comment:2 by , 14 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
I was about to reply linking to the 4.2 release notes then saw you answered your own question:
To add, the 4.2 release notes do say the following though:
...
But with the problem in this ticket, it seems squashing the migrations will be mandatory?
Yes :)
follow-up: 4 comment:3 by , 14 months ago
PS: If you disagree with the removal at the end of deprecation feel free to start a thread on the Django forum: https://www.djangoproject.com/community/
comment:4 by , 14 months ago
Replying to David Sanders:
PS: If you disagree with the removal at the end of deprecation feel free to start a thread on the Django forum: https://www.djangoproject.com/community/
Thanks! I don't mind the removal itself, though I wonder if the release notes should be updated to better reflect the fact that squashing the migrations will be mandatory? The current wording seems to suggest it's optional. That said, this ticket is probably enough as additional documentation about this issue if anyone else encounters it in the future.
comment:5 by , 2 months ago
And the release notes say, "The AlterIndexTogether
migration operation is now officially supported only for pre-Django 4.2 migration files. For backward compatibility reasons, it’s still part of the public API, and there’s no plan to deprecate or remove it, but it should not be used for new migrations. "
I'm not immediately seeing what purpose it serves if it crashes (see #35679). I think removing an index, e.g. AlterIndexTogether("Pony", None)
, still works, but I'm not sure this provides sufficient value to keep it around.
To add, the 4.2 release notes do say the following though:
But with the problem in this ticket, it seems squashing the migrations will be mandatory?