Django 5.1 - AlterIndexTogether raising TypeError: 'class Meta' got invalid attribute(s): index_together

Django Internals ORM
1

I’m upgrading an application from django 5.0.9 to django 5.1.3 and I have a few historical migrations which contain AlterIndexTogether operations.

According to the docs, these operations should work just fine:

AlterIndexTogether is 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.

However, when trying to run migrations, a TypeError is raised:
# TypeError: 'class Meta' got invalid attribute(s): index_together

(bear in mind that all Meta classes have already been updated to use indexes instead of index_together, so the error is coming from AlterIndexTogether).

There have been several bug reports about this, which have been closed with the suggestion that this can be fixed by squashing migrations, e.g.:

However, squashing migrations does not remove the AlterIndexTogether, so it does not help.

Is anyone else facing this issue? Have you found a real workaround? Should I open another bug ticket?

Many thanks in advance!

2

This sounds like a genuine bug to me. I haven’t looked at the code, but I’m wondering if we can change the implementation of AlterIndexTogether in 5.x to delegate to AddIndex and RemoveIndex?

1 Like
3

I also think that delegating to AddIndex and RemoveIndex is the way forward. The fact AlterIndexTogether was kept around while raising an unhelpful TypeError instead of producing some instructions about how the problem can be solved appears to have been an oversight?

We can’t have AlterIndexTogether.reduce to list[type[AddIndex] | type[RemoveIndex]] because it requires knowledge about the current project state and one of the flaw of AlterTogether operations is that they don’t treat indices as CRUD atoms so there’s no way for the optimization logic to know in which context (which indices exists prior to the optimization) the operation is being reduced. In other words, there’s no way to have squashmigrations get rid of AlterIndexTogether that I know of; the shim will have to be kept around for the foreseeable future.

We can definitely adapt AlterIndexTogether.state_forwards and .database_(forwards|backwards) to delegate to AddIndex and RemoveIndex by introspecting state and from_state though.

Whichever solution we decide to go forward with should serve as a validation for attempting the same deprecation path on unique_together -> constraints.

4

From looking at the code it appears that CreateModel is also affected as it happily accepts options={"index_together": ...} and result in a similar crash when trying to create the associated model when applying migrations.

In this case we can adapt reduce though to result in [CreateModel, AddIndex, ...] for each member of options["index_together"] as no reference to the model can exists before a CreateModel.

5

Does it help if I create a new bug ticket? Or is this sufficiently tracked already? Thanks for looking into this

6

@andres-holvi thanks for the offer but I think we should come up with an agreed upon solution here before re-opening one of the tickets you linked.

I had a shot at a PoC that should restore minimal support for index_together references in historical migrations (both in CreateModel and through AlterIndexTogether).

It appears that all that was needed was to avoid passing index_together when rendering models and adjust CreateModel and AlterTogetherOptionOperation to rely directly on ModelState.options["index_together"] instead.

Could you give a shot at the proposed changes against your historical migrations and a test database and see if it helps?

1 Like
7

Thank you! Using your branch I can confirm that both manage.py makemigrations and manage.py migrate run without crashing, so it definitely helped! The test DB seems to be working fine as well.

The result of replacing index_together = [("field_1", "field_2",)] by indexes = [models.Index(fields=("field_1", "field_2"))] seems to be a migration that renames the index, which seems reasonable:
~ Rename unnamed index for ('field_1', 'field_2') on model to app_model__a0d11d_idx

However, running manage.py migrate does not seem to apply that migration. I’m not sure if that is expected behaviour:

Running migrations:
  No migrations to apply.
8

The result of replacing index_together = [("field_1", "field_2",)] by indexes = [models.Index(fields=("field_1", "field_2"))] seems to be a migration that renames the index, which seems reasonable

That should be a pre-Django 5.1 behavior though as 5.1 doesn’t allow you to even declare Meta.unique_together. The proposed patch here should be applied on top of a 5.1 install for a project where the migrations that renames the index_together have already been generated (on Django < 5.1) per the deprecation guidelines.

However, running manage.py migrate does not seem to apply that migration.

Something doesn’t add up here, what does the generated migration file looks like and where is it created?

If I follow these instructions everything seems to work fine

  1. Create a virtual environment and install Django 4.2 in and create a model that make use of unique_together.
  2. Generate the migrations to simulate pre-Django 4.2 historical migrations
  3. Change the models to use indexes per the deprecation guidelines
  4. Upgrade to Django 5.1 and try to run migrate which results in TypeError: 'class Meta' got invalid attribute(s): index_together
  5. Apply the propose patch and notice that migrate now doesn’t crash but has nothing to apply understandably given we didn’t generate any new migrations.
  6. Drop the database and try a re-run from the beginning and notice that the sqlmigrate output for each migrations is the same.

Remember the reported issue here is that historical migrations generated prior to Django 4.2 that still contain usage of AlterIndexTogether or CreateModel(options["index_together"]) would crash on 5.1 not that users who didn’t follow the deprecation guidelines between 4.2 and 5.0 are not able to generate the proper migrations from 5.1.

9

My apologies, you can disregard what I said about the not-applied migration. I must have put the test DB in a weird state by doing several tests.

I have re-tested your branch from scratch and everything seems to work fine, the TypeError does not happen and thus migrate does not crash.

10

Hi again! How can I help moving this forward? Are we in need of more community discussion? I would really like to see a fix for this :slight_smile:

11

Hello @andres-holvi given we have support from @Lily-Foote and myself that this is a legitimate bug I think we should re-open #34856 and consider using the proposed PR changes to fix it and backport to 5.1.

I don’t think I’ll have much time to see the changes through in the next weeks so feel free to make a new PR out of it. The only part missing IMO is to include back some of the tests/migrations/test_operations.py tests removed in 2abf417c815c20f41c0868d6f66520b32347106e to ensure the solution is bullet proof.

3 Likes