Opened 14 months ago

Closed 14 months ago

Last modified 2 months ago

#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 run makemigrations
    • or run makemigrations before adding index_together and run it again so AlterIndexTogether operation is created, same result
  • Write a test that uses the database (a simple test with TestCase and not SimpleTestCase should suffice)
  • Optional: Update the model to use indexes instead of index_together and run makemigrations
  • Install latest Django main
  • Run python manage.py test -v3 and it fails when applying the migration that contains index_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 Sage Abdullah, 14 months ago

To add, the 4.2 release notes do say the following though:

Next, consider squashing migrations to remove index_together from historical migrations.
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. Use AddIndex and RemoveIndex operations instead.

But with the problem in this ticket, it seems squashing the migrations will be mandatory?

comment:2 by David Sanders, 14 months ago

Resolution: invalid
Status: newclosed

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

comment:3 by David Sanders, 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/

in reply to:  3 comment:4 by Sage Abdullah, 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 Tim Graham, 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.

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