Opened 15 months ago

Closed 9 days ago

Last modified 9 days ago

#34856 closed Bug (fixed)

Running tests with historical migrations that contain index together fails with TypeError.

Reported by: Sage Abdullah Owned by: Andrés Reverón Molina
Component: Migrations Version: dev
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes 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 (12)

comment:1 by Sage Abdullah, 15 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, 15 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, 15 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, 15 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, 4 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.

comment:6 by Andrés Reverón Molina, 6 weeks ago

Hi, I'm in the process of upgrading a project from Django 4.2.x to 5.1.x and I'm facing this issue.

Every instance of Meta.index_together has already been updated to use Meta.indexes , but there are still some AlterIndexTogether in historical migrations, which happily crash with a
TypeError: 'class Meta' got invalid attribute(s): index_together error.

Unfortunately squashing migrations does not help, as AlterIndexTogether is still there after the migrations have been squashed.

Is there a recommended workaround to this? Maybe what is suggested here https://code.djangoproject.com/ticket/35679#comment:1 ?

I think this ticket and https://code.djangoproject.com/ticket/35679 were maybe closed too soon.

Last edited 6 weeks ago by Andrés Reverón Molina (previous) (diff)

comment:7 by Tim Graham, 2 weeks ago

Resolution: invalid
Severity: NormalRelease blocker
Status: closednew
Triage Stage: UnreviewedAccepted

Reopening per Django forum thread.

comment:8 by Sarah Boyce, 2 weeks ago

Has patch: set
Owner: changed from nobody to Simon Charette
Status: newassigned

comment:9 by Sarah Boyce, 10 days ago

Owner: changed from Simon Charette to Andrés Reverón Molina

comment:10 by Sarah Boyce, 10 days ago

Triage Stage: AcceptedReady for checkin

comment:11 by Sarah Boyce <42296566+sarahboyce@…>, 9 days ago

Resolution: fixed
Status: assignedclosed

In b44efdfe:

Fixed #34856 -- Fixed references to index_together in historical migrations.

While AlterUniqueTogether has been documented to be still allowed in historical
migrations for the foreseeable future it has been crashing since 2abf417c815c20
was merged because the latter removed support for Meta.index_together which the
migration framework uses to render models to perform schema changes.

CreateModel(optionsunique_together) was also affected.

Refs #27236.

Co-authored-by: Simon Charette <charette.s@…>

comment:12 by Sarah Boyce <42296566+sarahboyce@…>, 9 days ago

In 2ee6ca6d:

[5.1.x] Fixed #34856 -- Fixed references to index_together in historical migrations.

While AlterUniqueTogether has been documented to be still allowed in historical
migrations for the foreseeable future it has been crashing since 2abf417c815c20
was merged because the latter removed support for Meta.index_together which the
migration framework uses to render models to perform schema changes.

CreateModel(optionsunique_together) was also affected.

Refs #27236.

Co-authored-by: Simon Charette <charette.s@…>

Backport of b44efdfe543c9b9f12690b59777e6b275cb08103 from main.

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